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/#review32458
-----------------------------------------------------------


Thanks for the new patch! This is really very nice work, looks good from my point of view! I just added a few minor nitpicks. Unless David has any objections, feel free to commit!

Even though the patch looks quite safe, I think it's better to push this to master only. This patch is a very welcome improvement and allows to fix another bug, but KDE 4.11 is quite close already, so there is no urgent need to push this to 4.10 and maybe risk a regression (in the unlikely event that there is a problem left that we have all overlooked).


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

    No space between method name and opening '('



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

    No space between method name and opening '('



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

    Does this need to be a QPointer? If the collection gets deleted, we hit an assert in DolphinRemoveAction::update() anyway. But I think it won't hurt much, so just leave it as it is if you wish.



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

    Dolphin coding style: put the ':' just behind the closing ')' (separeated by a space), and put the ',' behind QAction(parent).



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

    Not needed any more.



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

    Not needed any more.


- Frank Reininghaus


On May 13, 2013, 1:02 p.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:02 p.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/1e0d335c/attachment.htm>