Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/#review32497 --- This review has been submitted with commit 8e023ae9e5051cb7b81af86a178e37c1f2c5da94 by Dawit Alemayehu to branch master. - Commit Hook On May 14, 2013, 1:56 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/ --- (Updated May 14, 2013, 1:56 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
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/ --- (Updated May 14, 2013, 12:38 p.m.) Status -- This change has been marked as submitted. 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
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
--- 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
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
--- 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
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
On May 13, 2013, 9:16 a.m., Frank Reininghaus wrote: dolphin/src/dolphinpart.cpp, line 463 http://git.reviewboard.kde.org/r/108802/diff/4/?file=143452#file143452line463 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. That is my mistake. m_removeAction was supposed to be a QPointer. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/#review32426 --- 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
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
--- 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. Changes --- Here is the updated diff based on your feedback. There is only one slight behavioral change because of the removal of the parameter from update that allowed us to simulate the shiftKeyPressed state. I do not think this behavior change is worth moving the code that checked for Shift key press into the calling code is worth it. Anyhow, the behavior change is if a user presses both Shift keys while the context menu is visible, lets one of them go and presses it again. The action no longer changes where as before it did. To make it clear here is a description of what the behavior was previously: - Press one Shift key, action changes to Delete - Press the second Shift key while still holding down the first Shift key and the action goes back Move to Trash - Release the second Shift key, nothing changes. - Press the second shift key again and the action changes to Delete. The difference is that last behavior. With this code change, pressing the second Shift key does nothing until you release the first Shift key. 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 (updated) - 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
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
---
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
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/#review32467 --- dolphin/src/dolphinpart.cpp http://git.reviewboard.kde.org/r/108802/#comment24147 I don't understand the use of QPointer. Wouldn't it be enough to add m_removeAction = 0; after this line? The parent of the removeaction is this, so surely there's no other way for the action to be automatically deleted. So the QPointer doesn't seem useful to me. - David Faure 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
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
On May 13, 2013, 10:47 p.m., David Faure wrote: dolphin/src/dolphinpart.cpp, line 463 http://git.reviewboard.kde.org/r/108802/diff/5/?file=143487#file143487line463 I don't understand the use of QPointer. Wouldn't it be enough to add m_removeAction = 0; after this line? The parent of the removeaction is this, so surely there's no other way for the action to be automatically deleted. So the QPointer doesn't seem useful to me. That is laziness on my part. I wanted the QPointer to handle all of that for me. You are completely right that this overhead is unnecessary ; so I will fix it. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/#review32467 --- 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
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/ --- (Updated May 14, 2013, 1:56 a.m.) Review request for KDE Base Apps, David Faure and Frank Reininghaus. Changes --- Updated based on feedback. 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 (updated) - 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
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
On May 5, 2013, 10:16 p.m., David Faure wrote: dolphin/src/dolphinpart.cpp, line 623 http://git.reviewboard.kde.org/r/108802/diff/2/?file=142264#file142264line623 Same code as in dolphincontextmenu. If it can't be shared, at least there should be a comment about where the code comes from, for easier maintainance. I will look to see if this code can be refactored and shared between these two classes. On May 5, 2013, 10:16 p.m., David Faure wrote: dolphin/src/dolphinpart.cpp, line 664 http://git.reviewboard.kde.org/r/108802/diff/2/?file=142264#file142264line664 There are other ways to bring the context menu than the right button. There's the context key, too. So better watch for the QEvent::ContextMenu event instead of hardcoding release of right button, if it really has to be done with the event filter. QEvent::ContextMenu won't work. That is the first thing I tried. I think the context menu event is consumed by some widget that emits its own specialized signal signal (requestContextMenu). Anyhow, that does not matter. I realized it is not need in this instance. See updated patch. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/#review32111 --- On May 5, 2013, 1:53 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/ --- (Updated May 5, 2013, 1:53 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/dolphinpart.h 7881ded dolphin/src/dolphinpart.cpp 627ba79 Diff: http://git.reviewboard.kde.org/r/108802/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/ --- (Updated May 13, 2013, 1:35 a.m.) Review request for KDE Base Apps, David Faure and Frank Reininghaus. Changes --- - Factored out the Delete/Move To Trash action into own class, DolphinRemoveAction. - Updated both the DolphinPart and DolphinContextMenu to use this new DolphinRemoveAction class to manage Delete/Move to Trash actions. The only thing I am unsure of is whether or not the new shared action class should be added to libdolphinprivate or not. It seemed to me to be the logical place to put it since it is shared by both the Dolphin binary as well as the kpart. Otherwise, this seems to work well for both Konqueror as well as Dolphin. It is also much cleaner to look at. 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 (updated) - 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
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
--- 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. Changes --- Removed trailing white spaces from the newly added implementation/header files. 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 (updated) - 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
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/ --- (Updated May 5, 2013, 1:53 p.m.) Review request for KDE Base Apps, David Faure and Frank Reininghaus. Changes --- Update patch not to use KModifierKeyInfo. 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 (updated) - dolphin/src/dolphinpart.h 7881ded dolphin/src/dolphinpart.cpp 627ba79 Diff: http://git.reviewboard.kde.org/r/108802/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/#review32111 --- Thanks for taking care of this! Just a few comments. dolphin/src/dolphinpart.cpp http://git.reviewboard.kde.org/r/108802/#comment23909 This if() is not useful, delete NULL is fine. dolphin/src/dolphinpart.cpp http://git.reviewboard.kde.org/r/108802/#comment23910 Shouldn't this be done only if addDel is true, as it was before? dolphin/src/dolphinpart.cpp http://git.reviewboard.kde.org/r/108802/#comment23911 Same code as in dolphincontextmenu. If it can't be shared, at least there should be a comment about where the code comes from, for easier maintainance. dolphin/src/dolphinpart.cpp http://git.reviewboard.kde.org/r/108802/#comment23912 There are other ways to bring the context menu than the right button. There's the context key, too. So better watch for the QEvent::ContextMenu event instead of hardcoding release of right button, if it really has to be done with the event filter. - David Faure On May 5, 2013, 1:53 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/ --- (Updated May 5, 2013, 1:53 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/dolphinpart.h 7881ded dolphin/src/dolphinpart.cpp 627ba79 Diff: http://git.reviewboard.kde.org/r/108802/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
On Feb. 11, 2013, 9:38 a.m., Frank Reininghaus wrote: Thanks Dawit for working on this! Concerning your question: the state of the modifiers only matters when the context menu is open (or just about to be opened), right? Couldn't one move the m_keyInfo-related code from the constructor to slotOpenContextMenu() and disconnect from the signal again at the end of that function? Yes, you probably can get away with doing that in Dolphin. At least that way the slot won't be unnecessarily activated everytime a key is pressed elsewhere. Unfortunately, there is no such clean solution for the dolphinpart I am afraid, since we won't be able to know when the context menu closed. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/#review27187 --- On Feb. 6, 2013, 1:05 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/ --- (Updated Feb. 6, 2013, 1:05 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/dolphinpart.h 7881ded dolphin/src/dolphinpart.cpp 627ba79 Diff: http://git.reviewboard.kde.org/r/108802/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/#review27187 --- Thanks Dawit for working on this! Concerning your question: the state of the modifiers only matters when the context menu is open (or just about to be opened), right? Couldn't one move the m_keyInfo-related code from the constructor to slotOpenContextMenu() and disconnect from the signal again at the end of that function? - Frank Reininghaus On Feb. 6, 2013, 1:05 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/ --- (Updated Feb. 6, 2013, 1:05 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/dolphinpart.h 7881ded dolphin/src/dolphinpart.cpp 627ba79 Diff: http://git.reviewboard.kde.org/r/108802/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request 108802: Switch Delete/Move To Trash actions when Shift key is pressed in Konqueror context menu
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108802/ --- 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/dolphinpart.h 7881ded dolphin/src/dolphinpart.cpp 627ba79 Diff: http://git.reviewboard.kde.org/r/108802/diff/ Testing --- Thanks, Dawit Alemayehu
