Review Request 110342: Dolphin Places: Make it easier to drag and drop items

[Frank Reininghaus]

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

Ship it!


Thanks for the new patch!

Sorry about my point (a) - I got that wrong. I guess the readability would benefit (and my mistake wouldn't have happened) if we changed "if (!droppingBetweenItems)" -> "if (droppingBetweenItems)" and swapped the two branches, but I think we've annoyed you enough now ;-)

Feel free to push this to master - I would prefer not to commit this to the 4.10 branch because I can already see the future bug reports that dropping stuff on items is harder now. I'd like to use the feedback from 4.11 beta releases to see if we have to reduce the 30% margin to something slightly smaller to make everyone happy.

Oh, one little issue left: I see that you changed

m_autoActivationTimer->setProperty("index", index);

to

m_autoActivationTimer->setProperty("index", newHoveredWidget->index());

I guess this should be reverted?

Many thanks for this work! As you have found out, getting this right is far from trivial, which is probably the reason why the pre-4.9.0 hack has survived until now.


- Frank Reininghaus


On May 8, 2013, 8:24 a.m., Vishesh Handa wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110342/
> -----------------------------------------------------------
> 
> (Updated May 8, 2013, 8:24 a.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
>     Dolphin Places: Make it easier to drag and drop items
>     
>     When doing a drop, a check is performed to see if it is within x pixels
>     from the top or x pixel from the bottom of the rect. If it is, then the
>     drop is considered a drop between items.
>     
>     This x was fixed to qMax( 4, myStyleOption.padding ) which would
>     generally be 4. This is fine for some cases, but when the rectangle size
>     increases then this 4 pixels is not enough. Hence this 'x' is now being
>     set to 30% of the rectangle height.
>     
>     By default the rectangle height is 20 pixels, so x is now 6 instead of 4
>     in the default case, which does make it slightly easier.
>     
>     Also, this in-between-items check is only performed when moving from one
>     item to another. This is not good since if you enter the item and the
>     bottom, the indicator is shown, and then as to start moving it up it
>     stops showing, and then it should start showing again as you approach
>     the top edge.
>     
>     Modified the code to run the check on every mouse drag event even if the
>     hovered item has not changed.
>     
>     Both these changes combined make it much easier to drag and drop items.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kitemlistcontroller.cpp c6239df 
>   dolphin/src/kitemviews/kitemlistview.cpp a2629c5 
> 
> Diff: http://git.reviewboard.kde.org/r/110342/diff/
> 
> 
> Testing
> -------
> 
> Much easier
> 
> 
> Thanks,
> 
> Vishesh Handa
> 
>

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