Review Request: Another approach to fix bug 291068, be more permissive

Matěj Laitl matej at laitl.cz
Mon Feb 6 21:18:03 UTC 2012



> On Feb. 6, 2012, 9 a.m., Bart Cerneels wrote:
> > If it works as mentioned I like to see it committed. Can't test currently since my laptop is 60 km away and I'm stuck with an OSX macbook today ;).
> > 
> > The "forbidden" drop indicator can probably be implemented safer (and with less code) by grabbing the mouseEnter events and drawing the drop indicator differently. I tried changing the mouse pointer itself but that is a bigger hack because of X11 quirkiness.
> > 
> > But I'm also wondering about other alternatives to the "long drag" problem. How about dynamically moving the collection drop targets (headers) nearer to the mouse pointer?

> If it works as mentioned I like to see it committed.

It does. :-)

> The "forbidden" drop indicator can probably be implemented safer (and with less code) by grabbing the mouseEnter events and drawing the drop indicator differently. I tried changing the mouse pointer itself but that is a bigger hack because of X11 quirkiness.

I originally tried to implement it using a safer way, but after an hour or so of reading Qt code I came to conclusion that I would be unable to do that. The biggest roadblock is that the individial rows in collecion browser are AFAIK not widgets by itself - the whole View in the widget. That means that you get {mouse,drag}EnterEvents just once for the whole view. It is then particularly tricky to get the current drop target item, see QAbstractItemView::dragMoveEvent() (mouse cursor is determined by event->accept()/ignore()):

void QAbstractItemView::dragMoveEvent(QDragMoveEvent *event)
1884	{
(...)
1890	    // ignore by default
1891	    event->ignore();
1892	
1893	    QModelIndex index = indexAt(event->pos());
1894	    d->hover = index;
1895	    if (!d->droppingOnItself(event, index)
1896	        && d->canDecode(event)) {
1897	
1898	        if (index.isValid() && d->showDropIndicator) {
1899	            QRect rect = visualRect(index);
1900	            d->dropIndicatorPosition = d->position(event->pos(), rect, index);
1901	            switch (d->dropIndicatorPosition) {
1902	            case AboveItem:
1903	                if (d->isIndexDropEnabled(index.parent())) {
1904	                    d->dropIndicatorRect = QRect(rect.left(), rect.top(), rect.width(), 0);
1905	                    event->accept();
1906	                } else {
1907	                    d->dropIndicatorRect = QRect();
1908	                }
1909	                break;
1910	            case BelowItem:
1911	                if (d->isIndexDropEnabled(index.parent())) {
1912	                    d->dropIndicatorRect = QRect(rect.left(), rect.bottom(), rect.width(), 0);
1913	                    event->accept();
1914	                } else {
1915	                    d->dropIndicatorRect = QRect();
1916	                }
1917	                break;
1918	            case OnItem:
1919	                if (d->isIndexDropEnabled(index)) {
1920	                    d->dropIndicatorRect = rect;
1921	                    event->accept();
1922	                } else {
1923	                    d->dropIndicatorRect = QRect();
1924	                }
1925	                break;
1926	            case OnViewport:
1927	                d->dropIndicatorRect = QRect();
1928	                if (d->isIndexDropEnabled(rootIndex())) {
1929	                    event->accept(); // allow dropping in empty areas
1930	                }
1931	                break;
1932	            }
1933	        } else {
1934	            d->dropIndicatorRect = QRect();
1935	            d->dropIndicatorPosition = OnViewport;
1936	            if (d->isIndexDropEnabled(rootIndex())) {
1937	                event->accept(); // allow dropping in empty areas
1938	            }
1939	        }
1940	        d->viewport->update();
1941	    } // can decode

Where d::isIndexDropEnabled(index) is just { model->flags( index ) & Qt::ItemIsDropEnabled }

I therefore came to a conclusion that abusing CollectionTreeItemModel::flags() is the only way to implement this without copying a significant portion of Qt code.

> But I'm also wondering about other alternatives to the "long drag" problem. How about dynamically moving the collection drop targets (headers) nearer to the mouse pointer?

I can imagine something like that (preferrably implemented atop this patch), but remember that one of the use-case behind this patch was to see what music is already in the target collection during drag. But I must say I'm quite happy with state of things with this patch applied.


> On Feb. 6, 2012, 9 a.m., Bart Cerneels wrote:
> > src/browsers/CollectionTreeView.cpp, line 479
> > <http://git.reviewboard.kde.org/r/103856/diff/1/?file=48631#file48631line479>
> >
> >     Keeping this list-of-dragged-from-collection is a bit icky. If this list get's out-of-sync we can get some very nasty and hard to solve bugs.
> >     
> >     Is there a cheap way to get a collection-ptr from a track?
> >     If not I don't see an immediate solution either.

This is the ugly part of the patch. However I think the list cannot became outdated with respect to current usage - it is updated in CollectionTreeView::dragEnterEvent() and only affects Qt::ItemIsDropEnabled of the CollectionTreeItemModel::flags() which is only used in CollectionTreeView::dragMoveEvent() which is guaranteed to come after dragEnterEvent(). A bit fragile, I admit. Remember that dragEnterEvent() is only called when mouse enters whole collection browser, typically once per drag.

I see a couple of alternatives/optimisations:
 * AmarokMimeData could offer QSet<Collection*> trackSourceCollections() that would be updated automatically in AmarokMimeData::{add,set}Tracks(). In this case however I would keep it internally in QSet<QWeakPointer<Collection>> to prevent dangling ptrs, and this seems rather ugly. But loop over all tracks would be avoided in CollectionTreeView::dragEnterEvent() (but not in CollectionTreeItemModel::dropMimeData())
 * CollectionTreeItemModelBase::setDragSourceCollections() could walk all collections and set their IsDropEnabled flag instead of storing m_dragSourceCollections. But that would be perhaps even more ugly. (and QSet::contains() is O(1) operation)
 * I originally designed CollectionTreeItemModelBase::m_dragSourceCollections as QSet<void *> to denote these pointers should be never dereferenced. (and only compared against) I later abandoned it for not being nice either. But docstring would certainly help here, will do.

I'm not decided, perhaps none of the above is good enough to be implemented.


- Matěj


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


On Feb. 3, 2012, 11:58 a.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103856/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2012, 11:58 a.m.)
> 
> 
> Review request for Amarok and Bart Cerneels.
> 
> 
> Description
> -------
> 
> Another approach to fix bug 291068, be more permissive
> 
> Bart originally solved the bug by enabling track dropping only
> precisely on root collection rows. This is IMO too much restrictive
> as it prevents you to drag tracks between 2 large expanded collections
> without excessive scrolling. (something I would miss) Additionally,
> there was no visual indication that the drop will not be performed.
> 
> This is my try to rework it in a way that:
>  * keeps drops onto artists/albums (whatever you first level
>    entity in collection browser is) allowed. There is a drop indicator
>    that clearly shows that the drop will go _between_ the entities, not
>    to them.
>  * disables drops to read-only collections
>  * disabled drops are indicated visually using the not-allowed mouse
>    cursor (the tricky part, but commented well in code)
> 
> BUG: 291068
> 
> 
> This addresses bug 291068.
>     https://bugs.kde.org/show_bug.cgi?id=291068
> 
> 
> Diffs
> -----
> 
>   ChangeLog ed4e955deeebf6a506e87ca8973645b9eeadc67b 
>   src/browsers/CollectionTreeItemModel.cpp d2bab082f44b37fa8945c827006439e7deb0017e 
>   src/browsers/CollectionTreeItemModelBase.h c6cca46fff956f9123299cb29c81e6a792abbc3b 
>   src/browsers/CollectionTreeItemModelBase.cpp e7f8e620a0ae2f7546c879dfd5d67a13fcb0f34a 
>   src/browsers/CollectionTreeView.h b70f99d811d1f7f271ec79298c3b0140fbd0ede4 
>   src/browsers/CollectionTreeView.cpp c9069c770fd28fe16e76b1af132f3c7dff4c82d3 
> 
> Diff: http://git.reviewboard.kde.org/r/103856/diff/diff
> 
> 
> Testing
> -------
> 
> Works as described here.
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120206/1b9cc4eb/attachment.html>


More information about the Amarok-devel mailing list