<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>