D6281: Prevent folders from drag and dropping onto themselves in dolphin main view

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> draganddrophelpertest.cpp:29
> +    void testUrlListMatchesUrl();
> +    void testUrlListMatchesUrl_data();
> +};

Usually the `_data()` function goes above the test function.

> draganddrophelpertest.cpp:38
> +
> +    QVERIFY(DragAndDropHelper::urlListMatchesUrl(urlList, url) == expected);
> +}

`QCOMPARE` here

> draganddrophelpertest.cpp:45
> +    QTest::addColumn<QUrl>("url");
> +    QTest::addColumn<bool>("expected");
> +

Maybe "expectedMatch" ?

> draganddrophelpertest.cpp:47
> +
> +    QTest::newRow("1") << (QList<QUrl>() << QUrl("file:///root")) << QUrl("file:///root") << true;
> +    QTest::newRow("2") << (QList<QUrl>() << QUrl("file:///root/")) << QUrl("file:///root") << true;

A number is not very descriptive as name for a test case. Ideallt it should be a sentence describing what are we testing.

Also, please use `QUrl::fromLocalFile()` instead of hardcoding the `file://` scheme.

Personal taste: how about declaring the url lists with `QList<QUrl> { QUrl(...) }` ?

> draganddrophelpertest.cpp:50-52
> +    QTest::newRow("4") << (QList<QUrl>() << QUrl("/usr/share") << QUrl("/usr/local/bin")) << QUrl("/usr/bin") << false;
> +    QTest::newRow("5") << (QList<QUrl>() << QUrl("/usr/share") << QUrl("/usr/local/bin")) << QUrl() << false;
> +    QTest::newRow("6") << QList<QUrl>() << QUrl("/usr/bin") << false;

`QUrl::fromLocalFile()` also here

> draganddrophelpertest.cpp:58
> +#include "draganddrophelpertest.moc"
> \ No newline at end of file


Files should always end with a newline

REVISION DETAIL
  https://phabricator.kde.org/D6281

To: emateli, #dolphin, elvisangelaccio, ngraham, rkflx, dfaure
Cc: rkflx, ngraham, elvisangelaccio, dfaure, anthonyfieroni, #konqueror, spoorun, navarromorales, firef, andrebarros, emmanuelp
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20171107/f9569927/attachment.htm>