D8862: Extend KFilePlacesModel API

[David Faure]

dfaure added a comment.


  This seems to be bundling several unrelated additions into the same commit?
  Please have one commit per feature / change / addition.
  
  I know that phabricator makes it difficult to have one review per commit, but at least the commits should be separate.

INLINE COMMENTS

> kfileplacesmodeltest.cpp:893
> +
> +    QCOMPARE(expectedScheme, convertedUrl.scheme());
> +    QCOMPARE(expectedUrl, convertedUrl);

expectedScheme is part of the expectedUrl already, so I don't see much point in this separate data column and check.

> kfileplacesmodel.cpp:933
> +
> +    before = before == -1 ? d->items.count() : before;
> +

This would fit better in the if/else above.
Just do `before = d->items.count()` in the if (before == -1) case, no need for the ternary operator nor the no-op (before = before).

> kfileplacesmodel.cpp:935
> +
> +    if (row == before || row + 1 == before) {
> +        return false;

Can you explain this condition? I understand the idea of an early exit if nothing to do, but I'm surprised that two different values of `before` would lead to "nothing to do".
There is only one current position....

> kfileplacesmodel.h:52
> +        GroupRole = 0x0a5b64ee,
> +        IconNameRole = 0x00a45c00
>      };

///< @since 5.41

REPOSITORY
  R241 KIO

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

To: renatoo, dfaure, mwolff
Cc: mwolff, dfaure, ngraham, #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171121/8c3d1302/attachment-0001.html>