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