Review Request: Do not delete files if the user clicks 'Move to Trash' in the context menu while Shift is pressed

[David Faure]

> On Nov. 29, 2012, 11:42 a.m., Dawit Alemayehu wrote:
> > I am sure you did not test the scenario when the user RMB clicks an item and then presses the Shift-key before clicking on the "Move To Trash" action. IOW, if the user presses the Shift key before the RMB click, the correct action ("Delete") is shown in the context menu. Otherwise, the "Move To Trash" is shown. That is the scenario this code was supposed to handle. Simply put Shift+"Move To Trash" == Delete. And I venture to guess that the behavior was not change when the "Delete" action is enable by default for the sake of consistency.
> 
> Frank Reininghaus wrote:
>     Thanks for the quick reply. I did indeed not test that use case, but then I wonder if Shift-clicking the "Trash" action should really invoke a "Delete". Is that really what the user expects?
>     
>     Dolphin's context menu changes the action when Shift is pressed or released (provided that the 'Delete' action is not shown), so the user at least knows what will happen before clicking. Would that be an option for the DolphinPart as well?
> 
> Kai Uwe Broulik wrote:
>     I think the Trash option should never invoke a Delete. At the moment it is when I right click a file, there is the Trash option, when I then hold Shift it turns into the Delete option. When the Delete is always enabled, I think Shift should have no effect.
>     People that know the Shift keystroke would probably know that this Deletes a file, but if the label is "Move to Trash" it should do exactly that.
> 
> Dawit Alemayehu wrote:
>     Ahh. How can Dolphin's context menu change if the user presses the Shift key after the RMB click ? IOW, the context menu is already visible and by then the actions that are shown are already set. So the problem is not holding the Shift button and then clicking on an item with the RMB. It is the clicking on the item with the RMB first and once the context menu is present, pressing the Shift key because you actually wanted to delete the item. Simply put the user forgot to press the Shift key first.
>     
>
> 
> Kai Uwe Broulik wrote:
>     It does change. Right click, and then press Shift, see how the menu changes :)
> 
> Frank Reininghaus wrote:
>     @Dawit: the code for changing the action is in DolphinContextMenu::updateRemoveAction() in dolphincontextmenu.cpp. I wasn't aware of that either before reading that bug report ;-)
>     
>     But in any case, I think that changing the action while the context menu is already open and the Shift key is pressed or released is better than just invoking "Delete" when one Shift-clicks "Trash". Therefore, I asked if it might make sense to do that in the DolphinPart as well.

Yes, it would make sense to do the same in dolphinpart. Actually this changing of the text when pressing Delete was happening in konqueror's popupmenu before dolphinpart existed, but probably got lost in the refactoring...


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107509/#review22750
-----------------------------------------------------------


On Nov. 29, 2012, 8:06 a.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107509/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2012, 8:06 a.m.)
> 
> 
> Review request for Dolphin, Dawit Alemayehu and David Faure.
> 
> 
> Description
> -------
> 
> The problem occurs in Konqueror and Dolphin if the 'Delete' context menu entry is enabled, and Shift is pressed while Dolphin's context menu is opened. The menu shows both "Move To Trash" and "Delete" then, but clicking "Move to Trash" will also result in a "Delete".
> 
> The reason seems to be that not only the context menu, but also DolphinViewActionHandler::slotTrashActivated(Qt::MouseButtons, Qt::KeyboardModifiers modifiers) tries to be clever, checks if 'Shift' is pressed and deletes the file then. But I do not see why this is necessary. Both Dolphin's and the DolphinPart's context menu take care of the Shift issue themselves in different ways, and I don't see why an invocation of the "Move to Trash" action should ever need to delete the file. But maybe I've overlooked something, so I thought I'd better have some more people have a look at my patch before I accidentally break something else.
> 
> 
> This addresses bug 307254.
>     http://bugs.kde.org/show_bug.cgi?id=307254
> 
> 
> Diffs
> -----
> 
>   dolphin/src/views/dolphinviewactionhandler.cpp 0249964 
> 
> Diff: http://git.reviewboard.kde.org/r/107509/diff/
> 
> 
> Testing
> -------
> 
> Tested trashing and deleting files in Dolphin and Konqueror with the context menu, the menu, Delete/Shift+Delete key presses, with the 'Delete' action enabled/disabled, and everything seems to work OK for me.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20121211/a3c5d1d7/attachment.htm>