Commit 8a90627d authored by Matěj Laitl's avatar Matěj Laitl
Browse files

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)

Some more code comments not present in review request are added.

BUG: 291068
REVIEW: 103856
DIGEST: More natural drag'n'drop behaviour in collection browser
parent 88f7be63
......@@ -46,8 +46,8 @@ Version 2.6-Beta 1
* Do not double-transcode tracks when importing from an audio CD. (BR 263775)
* Allow ripping of CD tracks with special characters in name tranks to
Björn Steinbrink. (BR 224437)
* Don't show organize collection when dropping track on non-collection
entry in Collection Browser. (BR 291068)
* Don't allow tracks to be dropped to their own collection and to
non-writable collections; indicate the fact visually. (BR 291068)
* Fix scrollarea following keyboard navigation. (BR 259791)
* Fix crash when trying to save custom equalizer presets. (BR 286227)
* Fix crash due to my silly method order error. (BR 291968)
......
......@@ -97,12 +97,26 @@ CollectionTreeItemModel::setLevels( const QList<int> &levelType )
Qt::ItemFlags
CollectionTreeItemModel::flags( const QModelIndex &idx ) const
{
if( !idx.isValid() )
return 0;
Qt::ItemFlags flags = CollectionTreeItemModelBase::flags( idx );
//TODO: check for CollectionLocation::isWritable().
if( !idx.parent().isValid() )
return flags | Qt::ItemIsDropEnabled;
else
return flags;
if( idx.parent().isValid() )
return flags; // has parent -> not a collection -> no drops
// we depend on someone (probably CollectionTreeView) to call
// CollectionTreeItemModelBase::setDragSourceCollections() every time a drag is
// initiated or enters collection browser widget
CollectionTreeItem *item = static_cast<CollectionTreeItem*>( idx.internalPointer() );
Q_ASSERT(item->type() == CollectionTreeItem::Collection);
if( m_dragSourceCollections.contains( item->parentCollection() ) )
return flags; // attempt to drag tracks from the same collection, don't allow this (bug 291068)
if( !item->parentCollection()->isWritable() )
return flags; // not writeable, disallow drops
// all paranoid checks passed, tracks can be dropped to this item
return flags | Qt::ItemIsDropEnabled;
}
QVariant
......@@ -124,13 +138,11 @@ CollectionTreeItemModel::dropMimeData( const QMimeData *data, Qt::DropAction act
if( !parent.isValid() )
return false;
if( parent.isValid() && ( row != -1 && column != -1 ) )
return false; //only droppable on root (collection header) items.
CollectionTreeItem *item = static_cast<CollectionTreeItem*>( parent.internalPointer() );
Q_ASSERT(item->type() == CollectionTreeItem::Collection);
Collections::CollectionLocation *targetLocation = item->parentCollection()->location();
Collections::Collection *targetCollection = item->parentCollection();
Collections::CollectionLocation *targetLocation = targetCollection->location();
Q_ASSERT(targetLocation);
//TODO: accept external drops.
......@@ -149,6 +161,9 @@ CollectionTreeItemModel::dropMimeData( const QMimeData *data, Qt::DropAction act
foreach( Collections::Collection *sourceCollection, collectionTrackMap.uniqueKeys() )
{
if( sourceCollection == targetCollection )
continue; // should be already catched by ...Model::flags(), but hey
Collections::CollectionLocation *sourceLocation;
if( sourceCollection )
{
......@@ -160,9 +175,6 @@ CollectionTreeItemModel::dropMimeData( const QMimeData *data, Qt::DropAction act
sourceLocation = new Collections::FileCollectionLocation();
}
if( sourceLocation == targetLocation )
continue;
if( action == Qt::CopyAction )
{
sourceLocation->prepareCopy( collectionTrackMap.values( sourceCollection ),
......
......@@ -1170,6 +1170,12 @@ void CollectionTreeItemModelBase::itemAboutToBeDeleted( CollectionTreeItem *item
}
}
void
CollectionTreeItemModelBase::setDragSourceCollections( const QSet<Collections::Collection*> &collections )
{
m_dragSourceCollections = collections;
}
int
CollectionTreeItemModelBase::levelCategory( const int level ) const
{
......
......@@ -101,6 +101,11 @@ class AMAROK_EXPORT CollectionTreeItemModelBase : public QAbstractItemModel
void itemAboutToBeDeleted( CollectionTreeItem *item );
/**
* This should be called every time a drag enters collection browser
*/
void setDragSourceCollections( const QSet<Collections::Collection*> &collections );
signals:
void expandIndex( const QModelIndex &index );
void allQueriesFinished();
......@@ -163,6 +168,13 @@ class AMAROK_EXPORT CollectionTreeItemModelBase : public QAbstractItemModel
QSet<Collections::Collection*> m_expandedCollections;
QSet<Collections::Collection*> m_expandedSpecialNodes;
/**
* Contents of this set are undefined if there is no active drag 'n drop operation.
* Additionally, you may _never_ dereference pointers in this set, just compare
* them with other pointers
*/
QSet<Collections::Collection*> m_dragSourceCollections;
protected slots:
void startAnimationTick();
void loadingAnimationTick();
......
......@@ -470,6 +470,25 @@ void CollectionTreeView::keyPressEvent( QKeyEvent *event )
QTreeView::keyPressEvent( event );
}
void
CollectionTreeView::dragEnterEvent( QDragEnterEvent *event )
{
// We want to indicate to the user that dropping to the same collection is not possible.
// CollectionTreeItemModel therefore needs to know what collection the drag originated
// so that is can play with Qt::ItemIsDropEnabled in flags()
const AmarokMimeData *mimeData = qobject_cast<const AmarokMimeData*>( event->mimeData() );
if( mimeData ) // drag from within Amarok
{
QSet<Collections::Collection *> srcCollections;
foreach( Meta::TrackPtr track, mimeData->tracks() )
{
srcCollections.insert( track->collection() );
}
m_treeModel->setDragSourceCollections( srcCollections );
}
QAbstractItemView::dragEnterEvent( event );
}
void
CollectionTreeView::dragMoveEvent( QDragMoveEvent *event )
{
......
......@@ -69,6 +69,7 @@ class CollectionTreeView: public Amarok::PrettyTreeView
void mousePressEvent( QMouseEvent *event );
void mouseReleaseEvent( QMouseEvent *event );
void keyPressEvent( QKeyEvent * event );
void dragEnterEvent( QDragEnterEvent *event );
void dragMoveEvent( QDragMoveEvent *event );
void startDrag( Qt::DropActions supportedActions );
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment