Review Request: OpenWith - configuration dialog

[Andreas Pakulat]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106388/#review18744
-----------------------------------------------------------



plugins/openwith/openwithconfig.h
<http://git.reviewboard.kde.org/r/106388/#comment14856>

    This variable is badly names, the model is not a widget.



plugins/openwith/openwithconfig.cpp
<http://git.reviewboard.kde.org/r/106388/#comment14855>

    This should be using KDialog::Ok, since the dialog will be closed when clicking it. Apply is for situations where the config dialog is kept open but the settings are being applied.



plugins/openwith/openwithconfig.cpp
<http://git.reviewboard.kde.org/r/106388/#comment14857>

    This should use okClicked and then not use close() in the save() slot.



plugins/openwith/openwithconfig.cpp
<http://git.reviewboard.kde.org/r/106388/#comment14859>

    Why is the selection stored in a (not quite well named) variable? You could just query it in save(). Also the dialog should not allow to be closed with the checkbox unchecked and no item selected IMHO. So the ok button should be disabled accordingly.



plugins/openwith/openwithconfig.cpp
<http://git.reviewboard.kde.org/r/106388/#comment14858>

    This could be directly connected to the setDisabled slot of the services widget. No need for a separate slot here.



plugins/openwith/openwithconfig.ui
<http://git.reviewboard.kde.org/r/106388/#comment14852>

    Where does this locale setting come from and what consequences does it have?



plugins/openwith/openwithconfig.ui
<http://git.reviewboard.kde.org/r/106388/#comment14853>

    This layout is unecessary, you're merely nesting a vertical layout in another which does not make any sense.



plugins/openwith/openwithconfig.ui
<http://git.reviewboard.kde.org/r/106388/#comment14854>

    What does this option do? If its about opening the file embedded then why have a checkbox instead of listing the embedded editor plugins available. Last I looked at the code thats what was happening with the menu too.


- Andreas Pakulat


On Sept. 9, 2012, 11:36 a.m., Przemek Czekaj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106388/
> -----------------------------------------------------------
> 
> (Updated Sept. 9, 2012, 11:36 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> Added a configuration dialog to the context menu.
> 
> 
> This addresses bug 287764.
>     http://bugs.kde.org/show_bug.cgi?id=287764
> 
> 
> Diffs
> -----
> 
>   plugins/openwith/CMakeLists.txt 8742aeb 
>   plugins/openwith/openwithconfig.h PRE-CREATION 
>   plugins/openwith/openwithconfig.cpp PRE-CREATION 
>   plugins/openwith/openwithconfig.ui PRE-CREATION 
>   plugins/openwith/openwithplugin.h 809a0e8 
>   plugins/openwith/openwithplugin.cpp 69404ef 
> 
> Diff: http://git.reviewboard.kde.org/r/106388/diff/
> 
> 
> Testing
> -------
> 
> Compiled, and local install. Tested by hand.
> 
> 
> Screenshots
> -----------
> 
> OpenWith context menu
>   http://git.reviewboard.kde.org/r/106388/s/722/
> Openwith dialog - internal option
>   http://git.reviewboard.kde.org/r/106388/s/723/
> Open with - selectable list
>   http://git.reviewboard.kde.org/r/106388/s/724/
> 
> 
> Thanks,
> 
> Przemek Czekaj
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120909/78225582/attachment.html>