<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/103856/">http://git.reviewboard.kde.org/r/103856/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 6th, 2012, 9 a.m., <b>Bart Cerneels</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> 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.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 6th, 2012, 9 a.m., <b>Bart Cerneels</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/103856/diff/1/?file=48631#file48631line479" style="color: black; font-weight: bold; text-decoration: underline;">src/browsers/CollectionTreeView.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">CollectionTreeView::dragEnterEvent( QDragEnterEvent *event )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">479</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="n">AmarokMimeData</span> <span class="o">*</span><span class="n">mimeData</span> <span class="o">=</span> <span class="n">qobject_cast</span><span class="o"><</span><span class="k">const</span> <span class="n">AmarokMimeData</span><span class="o">*></span><span class="p">(</span> <span class="n">event</span><span class="o">-></span><span class="n">mimeData</span><span class="p">()</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Matěj</p>
<br />
<p>On February 3rd, 2012, 11:58 a.m., Matěj Laitl wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Amarok and Bart Cerneels.</div>
<div>By Matěj Laitl.</div>
<p style="color: grey;"><i>Updated Feb. 3, 2012, 11:58 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Works as described here.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=291068">291068</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>ChangeLog <span style="color: grey">(ed4e955deeebf6a506e87ca8973645b9eeadc67b)</span></li>
<li>src/browsers/CollectionTreeItemModel.cpp <span style="color: grey">(d2bab082f44b37fa8945c827006439e7deb0017e)</span></li>
<li>src/browsers/CollectionTreeItemModelBase.h <span style="color: grey">(c6cca46fff956f9123299cb29c81e6a792abbc3b)</span></li>
<li>src/browsers/CollectionTreeItemModelBase.cpp <span style="color: grey">(e7f8e620a0ae2f7546c879dfd5d67a13fcb0f34a)</span></li>
<li>src/browsers/CollectionTreeView.h <span style="color: grey">(b70f99d811d1f7f271ec79298c3b0140fbd0ede4)</span></li>
<li>src/browsers/CollectionTreeView.cpp <span style="color: grey">(c9069c770fd28fe16e76b1af132f3c7dff4c82d3)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103856/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>