Index: containments/desktop/plugins/folder/autotests/positionertest.h =================================================================== --- containments/desktop/plugins/folder/autotests/positionertest.h +++ containments/desktop/plugins/folder/autotests/positionertest.h @@ -52,8 +52,10 @@ void tst_defaultValues(); void tst_changeEnabledStatus(); void tst_changePerStripe(); + void tst_proxyMapping(); private: + QMap hash2map(const QHash &hash); void checkPositions(int perStripe); Positioner *m_positioner; Index: containments/desktop/plugins/folder/autotests/positionertest.cpp =================================================================== --- containments/desktop/plugins/folder/autotests/positionertest.cpp +++ containments/desktop/plugins/folder/autotests/positionertest.cpp @@ -29,6 +29,7 @@ #include "foldermodel.h" #include "positioner.h" +#include "screenmapper.h" QTEST_MAIN(PositionerTest) @@ -58,6 +59,9 @@ void PositionerTest::init() { m_folderModel = new FolderModel(this); + m_folderModel->setScreen(0); + m_folderModel->setScreenMapper(ScreenMapper::instance()); + m_folderModel->setUsedByContainment(true); m_positioner = new Positioner(this); m_positioner->setEnabled(true); m_positioner->setFolderModel(m_folderModel); @@ -209,6 +213,98 @@ QCOMPARE(s.count(), 2); } +void PositionerTest::tst_proxyMapping() +{ + auto *screenMapper = ScreenMapper::instance(); + FolderModel secondFolderModel; + secondFolderModel.setUrl(m_folderDir->path() + QDir::separator() + desktop ); + secondFolderModel.setUsedByContainment(true); + secondFolderModel.setScreenMapper(screenMapper); + secondFolderModel.setScreen(1); + Positioner secondPositioner; + secondPositioner.setEnabled(true); + secondPositioner.setFolderModel(&secondFolderModel); + secondPositioner.setPerStripe(3); + QSignalSpy s2(&secondFolderModel, &FolderModel::listingCompleted); + s2.wait(1000); + QSignalSpy s(m_folderModel, &FolderModel::listingCompleted); + + QMap expectedSource2ProxyScreen0; + QMap expectedProxy2SourceScreen0; + QMap expectedProxy2SourceScreen1; + QMap expectedSource2ProxyScreen1; + + for (int i = 0; i < m_folderModel->rowCount(); i++) { + expectedSource2ProxyScreen0[i] = i; + expectedProxy2SourceScreen0[i] = i; + } + + // swap items 1 and 2 in the positioner + m_positioner->move({1, 2, 2, 1}); + expectedSource2ProxyScreen0[1] = 2; + expectedSource2ProxyScreen0[2] = 1; + expectedProxy2SourceScreen0[1] = 2; + expectedProxy2SourceScreen0[2] = 1; + + auto savedSource2ProxyScreen0 = expectedSource2ProxyScreen0; + auto savedProxy2SourceScreen0 = expectedProxy2SourceScreen0; + + QCOMPARE(hash2map(m_positioner->proxyToSourceMapping()), expectedSource2ProxyScreen0); + QCOMPARE(hash2map(m_positioner->sourceToProxyMapping()), expectedProxy2SourceScreen0); + QCOMPARE(hash2map(secondPositioner.proxyToSourceMapping()), expectedProxy2SourceScreen1); + QCOMPARE(hash2map(secondPositioner.sourceToProxyMapping()), expectedSource2ProxyScreen1); + + const auto movedItem = m_folderModel->index(1, 0).data(FolderModel::UrlRole).toString(); + + // move the item 1 from source (now in position 2) to the second screen + screenMapper->addMapping(movedItem, 1); + s.wait(1000); + s2.wait(1000); + + expectedProxy2SourceScreen1[0] = 0; + expectedSource2ProxyScreen1[0] = 0; + expectedSource2ProxyScreen0.clear(); + expectedProxy2SourceScreen0.clear(); + for (int i = 0; i < m_folderModel->rowCount(); i++) { + // as item 1 disappeared, the mapping of all items after that are shifted + auto proxyIndex = (i <= 1) ? i : i + 1; + expectedSource2ProxyScreen0[proxyIndex] = i; + expectedProxy2SourceScreen0[i] = proxyIndex; + } + + QCOMPARE(hash2map(m_positioner->proxyToSourceMapping()), expectedSource2ProxyScreen0); + QCOMPARE(hash2map(m_positioner->sourceToProxyMapping()), expectedProxy2SourceScreen0); + QCOMPARE(hash2map(secondPositioner.proxyToSourceMapping()), expectedProxy2SourceScreen1); + QCOMPARE(hash2map(secondPositioner.sourceToProxyMapping()), expectedSource2ProxyScreen1); + + // move back the same item to the first screen + screenMapper->addMapping(movedItem, 0); + s.wait(1000); + s2.wait(1000); + + // nothing on the second screen + expectedSource2ProxyScreen1.clear(); + expectedProxy2SourceScreen1.clear(); + // first screen should look like in the beginning + expectedSource2ProxyScreen0 = savedSource2ProxyScreen0; + expectedProxy2SourceScreen0 = savedProxy2SourceScreen0; + + QCOMPARE(hash2map(m_positioner->proxyToSourceMapping()), expectedSource2ProxyScreen0); + QCOMPARE(hash2map(m_positioner->sourceToProxyMapping()), expectedProxy2SourceScreen0); + QCOMPARE(hash2map(secondPositioner.proxyToSourceMapping()), expectedProxy2SourceScreen1); + QCOMPARE(hash2map(secondPositioner.sourceToProxyMapping()), expectedSource2ProxyScreen1); +} + +QMap PositionerTest::hash2map(const QHash &hash) +{ + QMap map; + for (auto it = hash.constBegin(); it != hash.constEnd(); ++it) { + map[it.key()] = it.value(); + } + + return map; +} + void PositionerTest::checkPositions(int perStripe) { QSignalSpy s(m_positioner, &Positioner::positionsChanged); Index: containments/desktop/plugins/folder/foldermodel.cpp =================================================================== --- containments/desktop/plugins/folder/foldermodel.cpp +++ containments/desktop/plugins/folder/foldermodel.cpp @@ -149,25 +149,23 @@ /* * position dropped items at the desired target position - * delay this via queued connection, such that the row is available and can be mapped - * when we emit the move request * * TODO: push this somehow into the Positioner */ - connect(m_dirModel, &QAbstractItemModel::rowsInserted, + connect(this, &QAbstractItemModel::rowsInserted, this, [this](const QModelIndex &parent, int first, int last) { for (int i = first; i <= last; ++i) { - const auto index = m_dirModel->index(i, 0, parent); - const auto url = m_dirModel->itemForIndex(index).url(); + const auto idx = index(i, 0, parent); + const auto url = itemForIndex(idx).url(); auto it = m_dropTargetPositions.find(url.fileName()); if (it != m_dropTargetPositions.end()) { const auto pos = it.value(); m_dropTargetPositions.erase(it); setSortMode(-1); emit move(pos.x(), pos.y(), {url}); } } - }, Qt::QueuedConnection); + }); /* * Dropped files may not actually show up as new files, e.g. when we overwrite * an existing file. Or files that fail to be listed by the dirLister, or... @@ -959,6 +957,15 @@ } } +static bool isDropBetweenSharedViews(const QList &urls, const QUrl &folderUrl) +{ + for (const auto &url : urls) { + if (!folderUrl.isParentOf(url)) + return false; + } + return true; +} + void FolderModel::drop(QQuickItem *target, QObject* dropEvent, int row) { QMimeData *mimeData = qobject_cast(dropEvent->property("mimeData").value()); @@ -1033,6 +1040,38 @@ } + auto dropTargetFolderUrl = dropTargetUrl; + if (dropTargetFolderUrl.fileName() == QLatin1String(".")) { + // the target URL for desktop:/ is e.g. 'file://home/user/Desktop/.' + dropTargetFolderUrl = dropTargetFolderUrl.adjusted(QUrl::RemoveFilename); + } + + // use dropTargetUrl to resolve desktop:/ to the actual file location which is also used by the mime data + if (isDropBetweenSharedViews(mimeData->urls(), dropTargetFolderUrl)) { + /* QMimeData operates on local URLs, but the dir lister and thus screen mapper and positioner may + * use a fancy scheme like desktop:/ instead. Ensure we always use the latter to properly map URLs, + * i.e. go from file:///home/user/Desktop/file to desktop:/file + */ + auto mappableUrl = [this, dropTargetFolderUrl](const QUrl &url) -> QString { + QString mappedUrl = url.toString(); + if (dropTargetFolderUrl != m_dirModel->dirLister()->url()) { + const auto local = dropTargetFolderUrl.toString(); + const auto internal = m_dirModel->dirLister()->url().toString(); + if (mappedUrl.startsWith(local)) { + mappedUrl.replace(0, local.size(), internal); + } + } + return mappedUrl; + }; + setSortMode(-1); + for (const auto &url : mimeData->urls()) { + m_dropTargetPositions.insert(url.fileName(), dropPos); + m_screenMapper->addMapping(mappableUrl(url), m_screen, ScreenMapper::DelayedSignal); + } + m_dropTargetPositionsCleanup->start(); + return; + } + Qt::DropAction proposedAction((Qt::DropAction)dropEvent->property("proposedAction").toInt()); Qt::DropActions possibleActions(dropEvent->property("possibleActions").toInt()); Qt::MouseButtons buttons(dropEvent->property("buttons").toInt()); Index: containments/desktop/plugins/folder/positioner.h =================================================================== --- containments/desktop/plugins/folder/positioner.h +++ containments/desktop/plugins/folder/positioner.h @@ -76,6 +76,15 @@ int rowCount(const QModelIndex &parent = QModelIndex()) const override; int columnCount(const QModelIndex &parent = QModelIndex()) const override; +#ifdef BUILD_TESTING + QHash proxyToSourceMapping() const { + return m_proxyToSource; + } + QHash sourceToProxyMapping() const { + return m_sourceToProxy; + } +#endif + Q_SIGNALS: void enabledChanged() const; void folderModelChanged() const; @@ -128,6 +137,7 @@ QHash m_proxyToSource; QHash m_sourceToProxy; + bool m_beginInsertRowsCalled = false; // used to sync the amount of begin/endInsertRows calls }; #endif Index: containments/desktop/plugins/folder/positioner.cpp =================================================================== --- containments/desktop/plugins/folder/positioner.cpp +++ containments/desktop/plugins/folder/positioner.cpp @@ -25,6 +25,15 @@ #include +void ensureUnique(const QHash mapping) +{ + auto values = mapping.values(); + qSort(values); + auto uniqueValues = values.toSet().toList(); + qSort(uniqueValues); + Q_ASSERT(uniqueValues == values); +} + Positioner::Positioner(QObject *parent): QAbstractItemModel(parent) , m_enabled(false) , m_folderModel(nullptr) @@ -406,6 +415,7 @@ if (!toIndices.contains(from)) { m_proxyToSource.remove(from); + ensureUnique(m_proxyToSource); } updateMaps(to, sourceRow); @@ -422,6 +432,10 @@ const int newCount = rowCount(); if (newCount > oldCount) { + if (m_beginInsertRowsCalled) { + endInsertRows(); + m_beginInsertRowsCalled = false; + } beginInsertRows(QModelIndex(), oldCount, newCount - 1); endInsertRows(); } @@ -444,11 +458,19 @@ QHashIterator it(m_proxyToSource); + QSet uniqueName; + QSet uniqueId; + while (it.hasNext()) { it.next(); + Q_ASSERT(!uniqueId.contains(it.value())); + uniqueId.insert(it.value()); + const QString &name = m_folderModel->data(m_folderModel->index(it.value(), 0), FolderModel::UrlRole).toString(); + Q_ASSERT(!uniqueName.contains(name)); + uniqueName.insert(name); if (name.isEmpty()) { qDebug() << this << it.value() << "Source model doesn't know this index!"; @@ -459,6 +481,7 @@ positions.append(name); positions.append(QString::number(qMax(0, it.key() / m_perStripe))); positions.append(QString::number(qMax(0, it.key() % m_perStripe))); + } } @@ -507,14 +530,28 @@ if (m_enabled) { if (m_proxyToSource.isEmpty()) { if (!m_pendingPositions) { - emit beginInsertRows(parent, start, end); + beginInsertRows(parent, start, end); + m_beginInsertRowsCalled = true; initMaps(end + 1); } return; } + // When new rows are inserted, they might go in the beginning or in the middle. + // In this case we must update first the existing proxy->source and source->proxy + // mapping, otherwise the proxy items will point to the wrong source item. + int count = end - start + 1; + m_sourceToProxy.clear(); + for (auto it = m_proxyToSource.begin(); it != m_proxyToSource.end(); ++it) { + int sourceIdx = *it; + if (sourceIdx >= start) { + *it += count; + } + m_sourceToProxy[*it] = it.key(); + } + int free = -1; int rest = -1; @@ -535,6 +572,7 @@ int remainder = (end - rest); beginInsertRows(parent, firstNew, firstNew + remainder); + m_beginInsertRowsCalled = true; for (int i = 0; i <= remainder; ++i) { updateMaps(firstNew + i, rest + i); @@ -544,6 +582,8 @@ } } else { emit beginInsertRows(parent, start, end); + beginInsertRows(parent, start, end); + m_beginInsertRowsCalled = true; } } @@ -563,6 +603,8 @@ m_proxyToSource.remove(proxyRow); m_pendingChanges << createIndex(proxyRow, 0); } + ensureUnique(m_proxyToSource); + ensureUnique(m_sourceToProxy); QHash newProxyToSource; QHash newSourceToProxy; @@ -582,7 +624,9 @@ } m_proxyToSource = newProxyToSource; + ensureUnique(m_proxyToSource); m_sourceToProxy = newSourceToProxy; + ensureUnique(m_sourceToProxy); m_lastRow = -1; int newLast = lastRow(); @@ -614,7 +658,10 @@ if (!m_ignoreNextTransaction) { if (!m_pendingPositions) { - emit endInsertRows(); + if (m_beginInsertRowsCalled) { + endInsertRows(); + m_beginInsertRowsCalled = false; + } } else { applyPositions(); } @@ -688,8 +735,20 @@ void Positioner::updateMaps(int proxyIndex, int sourceIndex) { + // ensure we don't get duplicate mappings + const auto oldSourceIndex = m_proxyToSource.value(proxyIndex, -1); + const auto oldProxyIndex = m_sourceToProxy.value(sourceIndex, -1); + if (oldSourceIndex != -1) { + m_sourceToProxy.remove(oldSourceIndex); + } + if (oldProxyIndex != -1) { + m_proxyToSource.remove(oldProxyIndex); + } + m_proxyToSource.insert(proxyIndex, sourceIndex); m_sourceToProxy.insert(sourceIndex, proxyIndex); + ensureUnique(m_proxyToSource); + ensureUnique(m_sourceToProxy); m_lastRow = -1; }