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