mwolff requested changes to this revision. mwolff added a comment. This revision now requires changes to proceed.
I guess we should split this patch up into three parts: a) add unit test + the fix you found b) (if still required) enforce uniqueness in `Positoiner::updateMaps` + unit test c) support drag and drop INLINE COMMENTS > positionertest.cpp:219 > + auto *screenMapper = ScreenMapper::instance(); > + FolderModel secondFolderModel; > + secondFolderModel.setUrl(m_folderDir->path() + QDir::separator() + > desktop ); second? this is the first and only one in this test, no? > positionertest.cpp:229 > + QSignalSpy s2(&secondFolderModel, &FolderModel::listingCompleted); > + s2.wait(1000); > + QSignalSpy s(m_folderModel, &FolderModel::listingCompleted); `QVERIFY` the wait, also below > positionertest.cpp:232 > + > + QMap<int, int> expectedSource2ProxyScreen0; > + QMap<int, int> expectedProxy2SourceScreen0; why not use hashes here too, then you can compare those below directly without having to convert the returned hash to a map first? > positionertest.cpp:261 > + screenMapper->addMapping(movedItem, 1); > + s.wait(1000); > + s2.wait(1000); verify > positionertest.cpp:282 > + screenMapper->addMapping(movedItem, 0); > + s.wait(1000); > + s2.wait(1000); verify > positionertest.h:58 > private: > + QMap<int, int> hash2map(const QHash<int, int> &hash); > void checkPositions(int perStripe); could be a free function, no need to make this a member > positioner.cpp:28 > > +void ensureUnique(const QHash<int, int> mapping) > +{ do we still need this? I don't think so - I added this only for debugging purposes. should probably be part of a unit test now > positioner.cpp:461 > > + QSet<QString> uniqueName; > + QSet<int> uniqueId; should also be removed and covered by a unit test instead I think > positioner.cpp:738 > { > + // ensure we don't get duplicate mappings > + const auto oldSourceIndex = m_proxyToSource.value(proxyIndex, -1); this may be obsoleted by now, can you check whether it's still required? REVISION DETAIL https://phabricator.kde.org/D8850 To: amantia, mwolff Cc: broulik, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart