Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu

[Frank Reininghaus]

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


Thanks for the patch, this is greatly appreciated! I could not test it myself yet, but I have a few comments already.

(a) The way you determine the actual action in DolphinRemoveAction by setting/reading the objectName() looks a bit awkward. Wouldn't it be easier to add a member m_action to this class? The only situation in which I see a potential problem with this approach would be a deletion of the action after the assignment of m_action in update() and before the invocation of slotRemoveActionTriggered(), but I'm not sure if this can actually happen. But even then, I think that adding a member m_useDeleteAction or something like that would be better than setting the objectName().

(b) When I just look at the calls to DolphinRemoveAction::update(bool shiftKeyPressed) in the source, it's impossible to tell what this function actually does. I think it might be more readable if we rename it to setShiftKeyPressed(bool) or something like that, drop the default value for the bool parameter, and move the check "qApp->keyboardModifiers() & Qt::ShiftModifier" to DolphinRemoveAction's constructor, DolphinContextMenu::insertDefaultItemActions() and DolphinPart::slotOpenContextMenu(). If you think that having this check in three different locations is too clumsy, we could alternatively add a new parameterless method "update()", which is called from these locations and checks the modifier state.

Some more comments inline...


dolphin/src/dolphinpart.cpp
<http://git.reviewboard.kde.org/r/108802/#comment24101>

    I could not test it yet, but this looks like we have a dangling pointer m_removeAction after this and might get a crash the second time we end up here? Maybe add m_removeAction = 0; after the delete.



dolphin/src/dolphinremoveaction.cpp
<http://git.reviewboard.kde.org/r/108802/#comment24102>

    #include <QApplication>



dolphin/src/dolphinremoveaction.cpp
<http://git.reviewboard.kde.org/r/108802/#comment24103>

    #include <KLocalizedString>


- Frank Reininghaus


On May 13, 2013, 1:38 a.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108802/
> -----------------------------------------------------------
> 
> (Updated May 13, 2013, 1:38 a.m.)
> 
> 
> Review request for KDE Base Apps, David Faure and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> This patch fixes DolphinPart such that the "Delete/Move To Trash" actions are automatically toggled if the user presses the Shift key and allows  https://git.reviewboard.kde.org/r/107509/ to be applied.
> 
> The code is completely based on what Dolphin's context menu does. Even though this works as planned, I still have reservations about the use of KModifierKeyInfo since every key press event from any application is sent to the application that connects to its signals. In my code and unlike what is done in Dolphin's context menu, I try to mitigate the impact of that by ignoring the signal when the part does not have the focus. Still if there is a better way to capture key press events at the part level I would like to use that instead. Any ideas ?
> 
> 
> Diffs
> -----
> 
>   dolphin/src/CMakeLists.txt ffb232c 
>   dolphin/src/dolphincontextmenu.h 1c65fab 
>   dolphin/src/dolphincontextmenu.cpp 89a169f 
>   dolphin/src/dolphinpart.h 7881ded 
>   dolphin/src/dolphinpart.cpp 627ba79 
>   dolphin/src/dolphinremoveaction.h PRE-CREATION 
>   dolphin/src/dolphinremoveaction.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/108802/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130513/20683148/attachment.htm>