----------------------------------------------------------- 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 > >
