----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/#review32428 -----------------------------------------------------------
Thanks for the refactoring. A few comments. dolphin/src/dolphinpart.h <http://git.reviewboard.kde.org/r/108802/#comment24108> not used in this header dolphin/src/dolphinpart.cpp <http://git.reviewboard.kde.org/r/108802/#comment24110> I liked the fact that the earlier patch would only install an app event filter while the contextmenu was shown. For performance reasons. But if it's too hard to make sure the code is correct (installing and removing in all cases where the contextmenu is shown/hidden), then OK, the eventfilter just has to be fast. dolphin/src/dolphinpart.cpp <http://git.reviewboard.kde.org/r/108802/#comment24109> I would test event->type() in this if() too, to save work for all other kinds of events. Application event filters are called VERY VERY often. dolphin/src/dolphinpart.cpp <http://git.reviewboard.kde.org/r/108802/#comment24106> the extra parentheses are not needed [and would hide a nested assignment if there was one] dolphin/src/dolphinremoveaction.h <http://git.reviewboard.kde.org/r/108802/#comment24104> that that -> that dolphin/src/dolphinremoveaction.cpp <http://git.reviewboard.kde.org/r/108802/#comment24107> When called with false because we know shift is not pressed, this will still query keyboardModifiers. A bit strange. The method tries to handle both the case where we know the state of shift, and the case where we don't (but that's 3 states in total, not 2). keyboardModifiers() is fast (it's just an accessor for a member in qapp), so instead of my first idea (two methods), I would suggest to simply remove the boolean argument from this method. This will simplify the callers. They just call this (without args) whenever an update is needed. dolphin/src/dolphinremoveaction.cpp <http://git.reviewboard.kde.org/r/108802/#comment24105> Changing the object name at runtime is a bit surprising. Why not add a QAction* member var with a pointer to the current action? That's all the slot needs. On the other hand, the object name will be set by the collection when adding the action into it -- which allows for configurable shortcuts. So object names shouldn't change (see also our recent discussion on the viewmode actions in konqueror). - David Faure 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 > >
