D8850: WIP: support drag and drop between shared folder view containments

[Milian Wolff]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20171120/c6227570/attachment-0001.html>