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