D8862: Extend KFilePlacesModel API
mlaurent added inline comments.
INLINE COMMENTS
> kfileplacesmodel.cpp:77
> +if (day > 0) {
> +date += QString("-%1").arg(day, 2, 10, QChar('0'));
> +}
QStringLiteral("-%1").arg(...)
> kfileplacesmodel.cpp:96
> +timelineUrl = QUrl(timelinePrefix + timelineDateString(year,
> month) +
> + '/' + timelineDateString(year, month, day));
> +} else if (path.endsWith(QLatin1String("/thismonth"))) {
QLatin1Char('/') +
> kfileplacesmodel.cpp:142
> +} else {
> +qWarning() << "Invalid search url:" << url;
> +searchUrl = url;
too bad it doesn't provide debug category here in this lib.
(I will add it today or tomorrow)
but don't care about it
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8862
To: renatoo, dfaure, mwolff
Cc: mlaurent, mwolff, dfaure, ngraham, #frameworks
D8862: Extend KFilePlacesModel API
mlaurent added a comment. oh it was abandoned ! ok :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8862 To: renatoo, dfaure, mwolff Cc: mlaurent, mwolff, dfaure, ngraham, #frameworks
D8862: Extend KFilePlacesModel API
renatoo abandoned this revision. renatoo added a comment. This review was spited in small ones: https://phabricator.kde.org/D8943 https://phabricator.kde.org/D8944 https://phabricator.kde.org/D8945 https://phabricator.kde.org/D8946 https://phabricator.kde.org/D8947 https://phabricator.kde.org/D8948 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8862 To: renatoo, dfaure, mwolff Cc: mwolff, dfaure, ngraham, #frameworks
D8862: Extend KFilePlacesModel API
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kfileplacesmodel.cpp:913
>
> +bool KFilePlacesModel::movePlace(int row, int before)
> +{
This should get its own unittest, especially since there are lots of edge cases.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8862
To: renatoo, dfaure, mwolff
Cc: mwolff, dfaure, ngraham, #frameworks
D8862: Extend KFilePlacesModel API
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
D8862: Extend KFilePlacesModel API
renatoo updated this revision to Diff 22703. renatoo added a comment. Created a public function 'moveItem' to be used by external apps REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8862?vs=22673=22703 REVISION DETAIL https://phabricator.kde.org/D8862 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp To: renatoo, dfaure, mwolff Cc: mwolff, dfaure, ngraham, #frameworks
D8862: Extend KFilePlacesModel API
renatoo updated this revision to Diff 22673. renatoo added a comment. Added a new role to access icon name property REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8862?vs=22666=22673 REVISION DETAIL https://phabricator.kde.org/D8862 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp To: renatoo, dfaure, mwolff Cc: mwolff, dfaure, ngraham, #frameworks
D8862: Extend KFilePlacesModel API
renatoo updated this revision to Diff 22666. renatoo marked 4 inline comments as done. renatoo added a comment. Fixed code style Added more unit test REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8862?vs=22652=22666 REVISION DETAIL https://phabricator.kde.org/D8862 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp To: renatoo, dfaure, mwolff Cc: mwolff, dfaure, ngraham, #frameworks
D8862: Extend KFilePlacesModel API
renatoo updated this revision to Diff 22652. renatoo added a comment. Renamed function from convertUrl to convertedUrl REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8862?vs=22651=22652 REVISION DETAIL https://phabricator.kde.org/D8862 AFFECTED FILES src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp To: renatoo, dfaure, mwolff Cc: mwolff, dfaure, ngraham, #frameworks
D8862: Extend KFilePlacesModel API
renatoo updated this revision to Diff 22651. renatoo added a comment. Created new function convertUrl Added @since tag to new functions REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8862?vs=22510=22651 REVISION DETAIL https://phabricator.kde.org/D8862 AFFECTED FILES src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesmodel.h src/filewidgets/kfileplacesview.cpp To: renatoo, dfaure, mwolff Cc: mwolff, dfaure, ngraham, #frameworks
D8862: Extend KFilePlacesModel API
mwolff added a comment. the new API and functionality should be covered by tests, it isn't so far as far as I can see. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8862 To: renatoo, dfaure Cc: mwolff, dfaure, ngraham, #frameworks
D8862: Extend KFilePlacesModel API
mwolff requested changes to this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8862 To: renatoo, dfaure, mwolff Cc: mwolff, dfaure, ngraham, #frameworks
D8862: Extend KFilePlacesModel API
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kfileplacesmodel.cpp:281
> KFilePlacesItem *item = static_cast *>(index.internalPointer());
> -
> -if (!item->isDevice()) {
> -return item->bookmark();
> -} else {
> -return KBookmark();
> -}
> + return item->bookmark();
> }
Please double-check indentation, looks like 5 spaces.
> kfileplacesmodel.cpp:786
> +
> +QString onlyInApp = bookmark.metaDataItem(QStringLiteral("OnlyInApp"));
> +if (onlyInApp != appName) {
remove one of the two spaces after '='
Add const in front, while at it.
> kfileplacesmodel.cpp:787
> +QString onlyInApp = bookmark.metaDataItem(QStringLiteral("OnlyInApp"));
> +if (onlyInApp != appName) {
> +bookmark.setMetaDataItem(QStringLiteral("OnlyInApp"), appName);
Please swap these two arguments around. In all preceding if()s, you have new !=
old, while here it's old != new, makes reading more difficult.
> kfileplacesmodel.h:133
>int row, int column, const QModelIndex )
> Q_DECL_OVERRIDE;
> +void refresh() const;
>
Missing method documentation, including `@since 5.41`.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8862
To: renatoo, dfaure
Cc: dfaure, ngraham, #frameworks
D8862: Extend KFilePlacesModel API
renatoo retitled this revision from "Extend API" to "Extend KFilePlacesModel API". REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8862 To: renatoo Cc: #frameworks
