Review Request: Fixes Bug 293200 - Drag&drop files in dolphin doesn't preserve origin

[Frank Reininghaus]

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


Thanks for the patch! Some parts look good already - let's work on it to make sure that this bug will be gone in KDE 4.9.2 :-)

1. Nothing needs to be changed in DolphinView - the auto activation/auto expansion behaviour of DolphinView is good as it is IMHO.

2. The behaviour of the Folders Panel should always be the same (auto-expand folders, but never auto-activate them). Therefore, the changes in DolphinMainWindow and the 'readSettings()' slot are not needed either. You could just add m_controller->setExpandOnly(true) in FoldersPanel just after the controller is initialised.

3. I see that you removed the 'else' in KItemListController::slotAutoActivationTimeout(), but that can lead to unexpected behaviour. I can happen that a folder gets expanded and activated at the same time in Details View (you can see that it has been expanded by going 'back'). Better write 'else if (!m_expandOnly)'.

Finally (sorry, I know that this is really nit-picking, but I believe that this will keep the code more readable in the long term), I'd prefer to change the name of the 'expandOnly' property to make sure that one can see at first sight what it is about. It's closely related to 'autoActivationDelay', so it makes sense to give it a similar name, maybe 'autoActivationBehavior', which takes an enum type 'AutoActivationBehaviour' (similar to SelectionBehavior, look at kitemlistcontroller.h) that has the values 'ActivationAndExpansion' (the default) and 'ExpansionOnly' (which will be used for the Folders Panel).

Thanks again for your valuable contributions! Please don't be angry with me because of all my little suggestions for improvement - most of them are based on my experience working with code that is touched by many people and my desire to keep Dolphin's code maintainable and easy to understand.

- Frank Reininghaus


On Sept. 15, 2012, 2:15 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106452/
> -----------------------------------------------------------
> 
> (Updated Sept. 15, 2012, 2:15 p.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> Fixes Bug 293200 - Drag&drop files in dolphin doesn't preserve origin
> 
> Patch 106381 Comment #3:
> When "Open folders during drag operations" is enabled, two things happen, both in the DolphinView and in the Folders Panel:
> 
> 1) When hovering a folder that can be expanded (this is the case for folders with sub-folders in the Folders Panel and in the DolphinView if in Details View mode), toggle its "expanded" state.
> 2) When hovering a folder that can not be expanded (i.e., a folder without sub-folders or any folder in Icons or Compact View), open this folder in the DolphinView via the KItemListController's itemActivated(int) signal.
> 
> The bug described in bug 293200 comment 3 is that 1) is always wanted, but 2) is not wanted for the Folders Panel.
> 
> 
> This addresses bug 293200.
>     http://bugs.kde.org/show_bug.cgi?id=293200
> 
> 
> Diffs
> -----
> 
>   dolphin/src/dolphinmainwindow.cpp 3bf3b3f 
>   dolphin/src/kitemviews/kitemlistcontroller.h a881526 
>   dolphin/src/kitemviews/kitemlistcontroller.cpp 8795481 
>   dolphin/src/panels/folders/folderspanel.h 14d8e87 
>   dolphin/src/panels/folders/folderspanel.cpp 0760200 
>   dolphin/src/views/dolphinview.cpp 20edd61 
> 
> Diff: http://git.reviewboard.kde.org/r/106452/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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