D8332: Added baloo urls into places model
ngraham added a comment. I'm not at a Linux machine right now. @renatoo, would you mind investigating that in a few non-Dolphin apps' file pickers and open and save dialogs? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: rkflx, mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
rkflx added a comment. Before the ball gets rolling, it would be nice if someone familiar with that part of the code could comment on what's going on with what I mentioned or can reproduce at least (it sounds like a generic problem to me): > Also some entries show an error message or are broken / show nothing at all (while the same entry works fine in Dolphin). --- (My previous comment was not ordered by priority yet, sorry for that…) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: rkflx, mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ngraham added a comment. Thanks for that investigation, @rkflx. I aspire to your level of thoroughness. macOS file pickers have a vertical scrollbar by default to show nearly equivalent content, so I don't think that's a huge deal. We could perhaps hide some sections by default (as long as it's obvious they're hidden and users have an easy and visible way to get them back), and I agree that the Devices section is probably more useful to more people than the Search section. We could consider re-ordering them. And you make a good point that some of the search sections aren't relevant to all apps. @renatoo seems to have a reasonable idea on that front. @renatoo, it would be even lovelier if you could also submit patches for affected apps to use the new flag once you've written that part. It's worth mentioning that not even macOS does this; the Open dialog for a text editor shows search options for pictures, music, and videos, for example. We could leapfrog Apple usability-wise here. :) Do you guys want to open some Bugzilla tickets to track the pieces of this work, or are we gonna get going right now? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: rkflx, mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo added a comment. In https://phabricator.kde.org/D8332#179633, @rkflx wrote: > This change caused a little bit of fallout for #Gwenview. Apparently the review focussed more on the code, but less so on the behaviour in users of the class. I'm not complaining, but given one of our focus goals is on usability and quality of the basic apps, it would be great if: > > - changes were tested more broadly in the future in addition to only looking at the code > - there was some help to fix the fallout > > Please head over to https://bugs.kde.org/show_bug.cgi?id=387824 if you can help Gwenview, thanks! > > --- > > In addition to Gwenview I also looked on lxr and did some testing based on what I found: > - The sidebar in `KDirSelectDialog` is now awful to use, because > - The devices entry (which for some users is more important/useful/frequently used than the search entries) is hidden from view (bad) and requires scrolling (annoying). → We should discuss reordering or (even better) adding collapsing and then collapsing some groups by default. > - The additional scrollbar makes the sidebar so small that you can't read most of the entries. → Add splitter and improve default width. > - To a lesser extent, this also applies to the normal file dialog (no scrollbar by default would be nice). > - Filesystem sidebars in https://phabricator.kde.org/tag/kdevelop/, #Okteta, https://phabricator.kde.org/tag/kile/, https://phabricator.kde.org/tag/kate/ and https://phabricator.kde.org/tag/krusader/: Some of the entries do not make sense in some of those apps at all, e.g. Videos/Images/…. Also some entries show an error message or are broken / show nothing at all (while the same entry works fine in Dolphin). > - That's it at first sight, luckily ;) > > Would be nice to fix those too… Let me know what's the best way forward here, i.e. what are Frameworks issues and where we'd need bugs filed against individual apps. I agree with you comments some entries in the place model does not make sense for some app. What I suggest is create a new function in the model that could filter the entries based on the context of the app. (Docs, Video, Music, Picture) and you can set a flag with any combination of this value. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: rkflx, mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
rkflx added a comment. This change caused a little bit of fallout for #Gwenview. Apparently the review focussed more on the code, but less so on the behaviour in users of the class. I'm not complaining, but given one of our focus goals is on usability and quality of the basic apps, it would be great if: - changes were tested more broadly in the future in addition to only looking at the code - there was some help to fix the fallout Please head over to https://bugs.kde.org/show_bug.cgi?id=387824 if you can help Gwenview, thanks! --- In addition to Gwenview I also looked on lxr and did some testing based on what I found: - The sidebar in `KDirSelectDialog` is now awful to use, because - The devices entry (which for some users is more important/useful/frequently used than the search entries) is hidden from view (bad) and requires scrolling (annoying). → We should discuss reordering or (even better) adding collapsing and then collapsing some groups by default. - The additional scrollbar makes the sidebar so small that you can't read most of the entries. → Add splitter and improve default width. - To a lesser extent, this also applies to the normal file dialog (no scrollbar by default would be nice). - Filesystem sidebars in https://phabricator.kde.org/tag/kdevelop/, #Okteta, https://phabricator.kde.org/tag/kile/, https://phabricator.kde.org/tag/kate/ and https://phabricator.kde.org/tag/krusader/: Some of the entries do not make sense in some of those apps at all, e.g. Videos/Images/…. Also some entries show an error message or are broken / show nothing at all (while the same entry works fine in Dolphin). - That's it at first sight, luckily ;) Would be nice to fix those too… Let me know what's the best way forward here, i.e. what are generic issues and where we'd need bugs filed against individual apps. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: rkflx, mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
This revision was automatically updated to reflect the committed changes. Closed by commit R241:7eb6333bdb48: Added baloo urls into places model (authored by Renato Araujo Oliveira Filho, committed by ngraham). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=22810&id=22942 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/CMakeLists.txt autotests/kfileplacesmodeltest.cpp autotests/kfileplacesviewtest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo marked an inline comment as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ervin accepted this revision. ervin added inline comments. INLINE COMMENTS > renatoo wrote in kfileplacesmodel.cpp:68 > This code came from dolphin. And it does not check at runtime for changes on > the configuration. > > But yes I agree keep track of this will be nice, I thought about that while > implementing it, but I did not find a way to track it in KConfing API > documentation, and since dolphin do not do that, I understand that is not > possible. > > Do you know if that is possible? What do you suggest? const is fine then. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ngraham added a comment. I'm going to land this on 11/25/17 unless I hear any more objections from anyone. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
dfaure accepted this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo added a comment. @dvratil, @ervin any comments? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 22810. renatoo marked an inline comment as done. renatoo added a comment. Renamed test function REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=22775&id=22810 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/CMakeLists.txt autotests/kfileplacesmodeltest.cpp autotests/kfileplacesviewtest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
dfaure added inline comments. INLINE COMMENTS > kfileplacesviewtest.cpp:70 > + > +void KFilePlacesViewTest::testUrlChangedContaisConvertedUrl_data() > +{ typo: Contains. I would just have called it `testUrlChanged` because it also checks that it's emitted, not just what the emitted value is :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 22775. renatoo added a comment. Updated from master REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=22763&id=22775 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/CMakeLists.txt autotests/kfileplacesmodeltest.cpp autotests/kfileplacesviewtest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 22763. renatoo marked 2 inline comments as done. renatoo added a comment. Updated unit test name REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=22451&id=22763 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/CMakeLists.txt autotests/kfileplacesmodeltest.cpp autotests/kfileplacesviewtest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
dfaure accepted this revision. dfaure added inline comments. INLINE COMMENTS > kfileplacesviewtest.cpp:56 > +// Ensure we'll have a clean bookmark file to start > +QFile::remove(bookmarksFile()); > + You could just call cleanupTestCase() here, I usually do that too. Cleanup at start, cleanup at end. > kfileplacesviewtest.cpp:102 > + > +void KFilePlacesViewTest::testConvertUrl() > +{ Isn't this rather a test for urlChanged()? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo added a dependent revision: D8943: Create 'KFilePlacesModel::convertedUrl' static function. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
mwolff accepted this revision. mwolff added a comment. lgtm from my side REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure, mwolff Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ngraham added a comment. @ervin and @dvratil, I'd like to land this in one week, since it's an open dependency blocking a lot of great work in Dolphin. Please respond if you have any remaining concerns, and preferably update your status if you don't. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo added a dependent revision: D8862: Extend API. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ngraham added a comment. @ervin and @dvratil, any remaining concerns? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo added a dependent revision: D8855: Use Kio::KPlacesModel as source model for PlacesItemModel. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo marked an inline comment as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 22451. renatoo added a comment. Make 'isFileIndexingEnabled' a static function REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=22328&id=22451 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/CMakeLists.txt autotests/kfileplacesmodeltest.cpp autotests/kfileplacesviewtest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
mwolff added a comment. lgtm, one minor nit, potentially for the future INLINE COMMENTS > kfileplacesmodel.cpp:967 > > +bool KFilePlacesModel::Private::isFileIndexingEnabled() const > +{ this could/should be a free function, not a member, considering its result is cached in a member variable REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure Cc: mwolff, dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ngraham accepted this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure Cc: dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 22328. renatoo marked 2 inline comments as done. renatoo added a comment. Fallback to initial url if the searchUrl is invalid. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=21932&id=22328 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/CMakeLists.txt autotests/kfileplacesmodeltest.cpp autotests/kfileplacesviewtest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure Cc: dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
dfaure added inline comments. INLINE COMMENTS > dvratil wrote in kfileplacesmodel.cpp:153 > typo: `withBaloo` Minor: QLatin1String("true") is enough for a comparison (no need to use a 16-bit-per-char string) > kfileplacesview.cpp:1350 > +qWarning() << "Invalid search url:" << url; > +Q_ASSERT(false); > +} So if I, as a user, manually add "search:/doesnotexist" as a bookmark in the places view, the application asserts and dies? If I'm right, then this doesn't seem wise. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure Cc: dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ngraham added a reviewer: dfaure. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent, dfaure Cc: dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ngraham added a comment. @ervin and @dvratil, any remaining concerns? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent Cc: dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 21932. renatoo marked an inline comment as done. renatoo added a comment. Fixed import REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=21807&id=21932 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/CMakeLists.txt autotests/kfileplacesmodeltest.cpp autotests/kfileplacesviewtest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent Cc: dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
dfaure added inline comments. INLINE COMMENTS > kfileplacesviewtest.cpp:30 > + > +#include > +#include Please don't include the full module (which also contains all QtCore) #include is OK, #include is not. Confusing, I know. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent Cc: dfaure, ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ngraham added a comment. @ervin and @dvratil, any remaining concerns? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
mlaurent accepted this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo marked an inline comment as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 21807. renatoo added a comment. Added warning message for invalid search:// urls REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=21758&id=21807 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/CMakeLists.txt autotests/kfileplacesmodeltest.cpp autotests/kfileplacesviewtest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ngraham added a comment. @mlaurent would you mind changing your approval status then? :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
usta added inline comments. INLINE COMMENTS > kfileplacesview.cpp:1348 > +searchUrl = searchUrlForType(QStringLiteral("Video")); > +} > + won't be nice to add an else statement? (for the sake of searchUrl ) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
mlaurent added a comment. +1 for me REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 21758. renatoo added a comment. Fixed code style REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=21647&id=21758 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/CMakeLists.txt autotests/kfileplacesmodeltest.cpp autotests/kfileplacesviewtest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo marked 5 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
mlaurent requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin, mlaurent Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
mlaurent added inline comments. INLINE COMMENTS > kfileplacesviewtest.cpp:2 > +/* This file is part of the KDE project > +Copyright (C) 2007 Kevin Ottens > + ? I don't think that you are kevin ottens :) and we are in 2017 :) > kfileplacesviewtest.cpp:20 > + > +#include > +#include QtCore/ is not necessary > kfileplacesviewtest.cpp:30 > + > +#include > +#include QtTest/ not necessart too > kfileplacesviewtest.cpp:70 > +} > +void KFilePlacesViewTest::testConvertUrl_data() > +{ new line before it > kfileplacesviewtest.cpp:111 > + > +QSignalSpy urlChangedSpy(&pv, SIGNAL(urlChanged(QUrl))); > +const QModelIndex targetIndex = pv.model()->index(row, 0); use new connect api for QSignalSpy REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ngraham added a comment. @dvratil and @ervin, any more concerns? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo added a dependent revision: D8434: Created 'remote' section. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo marked 4 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 21647. renatoo marked 2 inline comments as done. renatoo added a comment. Created unittest for PlacesView::convertUrl Refactory some small part of the code REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=21583&id=21647 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/CMakeLists.txt autotests/kfileplacesmodeltest.cpp autotests/kfileplacesviewtest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo added inline comments. INLINE COMMENTS > ervin wrote in kfileplacesmodel.cpp:68 > Do we really want to keep that state? It's never reevaluated so could be a > const if we keep it. > > Asks the question of what happens if the setting changes at runtime though. This code came from dolphin. And it does not check at runtime for changes on the configuration. But yes I agree keep track of this will be nice, I thought about that while implementing it, but I did not find a way to track it in KConfing API documentation, and since dolphin do not do that, I understand that is not possible. Do you know if that is possible? What do you suggest? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ervin requested changes to this revision. ervin added a comment. This revision now requires changes to proceed. All the fiddling with URLs makes me wonder if that wouldn't be better done on the KIO implementations side... but that's out of scope for that patch I think. INLINE COMMENTS > kfileplacesmodeltest.cpp:98 > > +// disable baloo > +KConfig config(QStringLiteral("baloofilerc")); Shouldn't we have tests verifying the extra URLs are properly inserted when baloo is enabled? Seems like it's trying to avoid testing the behavior change coming from the rest of the commit. > kfileplacesmodel.cpp:68 > + bookmarkManager(nullptr), > + fileIndexingEnabled(isFileIndexingEnabled()) > +{ Do we really want to keep that state? It's never reevaluated so could be a const if we keep it. Asks the question of what happens if the setting changes at runtime though. > kfileplacesmodel.cpp:499 > +bool allowedHere = appName.isEmpty() || (appName == > QCoreApplication::instance()->applicationName()); > +bool allowBalooUrl = isBalooUrl(url) ? fileIndexingEnabled : true; > + I find that naming confusing I mean, semantically fileIndexingEnabled would be your "allowBalooUrl". Could we come up with something better? Like "isSupportedUrl" perhaps? This is what it is about somewhat, we support all schemes except for the baloo ones depending on fileIndexingEnabled. > kfileplacesview.cpp:1301 > +const QString path = url.toDisplayString(QUrl::PreferLocalFile); > +if (path.endsWith(QLatin1String("yesterday"))) { > +const QDate date = QDate::currentDate().addDays(-1); Don't you want to match /yesterday and so on here? Like you're matching /documents and so on for the method below. > kfileplacesview.cpp:1329 > +if (day > 0) { > +date = date.arg(QString("-%1").arg(day, 2, 10, QChar('0'))); > +} else { This is unusual, why not concatenate + arg() in that case instead of the nested arg calls? Would allow you to drop the else part too. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 21583. renatoo added a comment. Updated parent branch REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=21227&id=21583 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
mlaurent added a dependent revision: D8367: Hidding place groups implementation in KFilePlacesModel. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 21227. renatoo added a comment. Upated parent branch REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=20952&id=21227 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo added a comment. In https://phabricator.kde.org/D8332#158392, @ngraham wrote: > @dvratil, any remaining objections? This looks great to me. > > @renatoo, once this is in, can we remove the now-duplicated code from Dolphin? Yes the idea is to try to use at least the model on Dolphin. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ngraham accepted this revision. ngraham added a comment. @dvratil, any remaining objections? This looks great to me. @renatoo, once this is in, can we remove the now-duplicated code from Dolphin? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ngraham added a comment. @dvratil, any remaining objections? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 20952. renatoo added a comment. Updated parent branch REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=20940&id=20952 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 20940. renatoo edited the summary of this revision. renatoo added a comment. Updated parent branch REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=20918&id=20940 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
ngraham added a reviewer: VDG. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo edited the summary of this revision. renatoo added a dependency: D8243: Implement support for categories on KfilesPlacesView. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo added a dependent revision: D8348: Add a section for removable devices. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo marked an inline comment as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo marked an inline comment as not done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo marked an inline comment as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 20918. renatoo added a comment. Typo fixed REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=20906&id=20918 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
usta added inline comments. INLINE COMMENTS > kfileplacesmodel.cpp:151 > > +// if baloo is enabled, add new ulrs even if the bookmark file is not > empty > +if (d->fileIndexingEnabled && typo: ulrs REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil Cc: usta, mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo marked 2 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil Cc: mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 20906. renatoo added a comment. Fixed typos Use QStringLiteral when possible REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=20905&id=20906 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil Cc: mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 20905. renatoo marked 5 inline comments as done. renatoo added a comment. Fixed typos REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=20879&id=20905 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil Cc: mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
mlaurent added inline comments. INLINE COMMENTS > kfileplacesview.cpp:1303 > +const int day = date.day(); > +timelineUrl = QUrl("timeline:/" + timelineDateString(year, month) + > + '/' + timelineDateString(year, month, day)); Add QStringLiteral and QLatin1Char('/') thanks REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil Cc: mlaurent, dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
dvratil requested changes to this revision. dvratil added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kfileplacesmodel.cpp:151 > > +// if baloo is enabled, add new ulrs even if the bookbark file is not > empty > +if (d->fileIndexingEnabled && typo: `bookbark` > kfileplacesmodel.cpp:153 > +if (d->fileIndexingEnabled && > +root.metaDataItem(QStringLiteral("withBaloon")) != > QStringLiteral("true")) { > + typo: `withBaloo` > kfileplacesmodel.cpp:155 > + > +root.setMetaDataItem("withBaloon", "true"); > +KFilePlacesItem::createSystemBookmark(d->bookmarkManager, typo: `withBaloo` > kfileplacesview.cpp:1321 > +{ > +QString date = QString::number(year) + '-'; > +if (month < 10) { You can use the 'fill' feature of `QString::arg()` to achieve the same thing with cleaner code: QString("%1-%2-%3").arg(year).arg(month, 2, 10, 0).arg(day, 2, 10, 0); > kfileplacesview.cpp:1344 > + > +if (path.endsWith(QLatin1String("documents"))) { > +searchUrl = searchUrlForType(QStringLiteral("Document")); You should probably match by `/documents` so that you don't match invalid paths (`search:/foodocuments`) > kfileplacesview.cpp:1360 > +{ > +static const QString jsonQuery("{\"dayFilter\": 0,\ > + \"monthFilter\": 0, \ use `QStringLiteral` for `jsonQuery`, then it won't have to be static REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil Cc: dvratil, ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 20879. renatoo added a comment. Does not load baloo urls if file index is disabled REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=20873&id=20879 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo, #frameworks, #dolphin, #kde_applications Cc: ngraham, #frameworks
D8332: Added baloo urls into places model
ngraham added reviewers: Frameworks, Dolphin, KDE Applications. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications Cc: ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo updated this revision to Diff 20873. renatoo added a comment. Removed baloo dependency REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8332?vs=20859&id=20873 REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo Cc: ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo Cc: ngraham, #frameworks
D8332: Added baloo urls into places model
renatoo planned changes to this revision. renatoo added a comment. Remove baloon dependency using KConfig API to query if it is enable, and hard-code search url with a fixed string. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo Cc: #frameworks
D8332: Added baloo urls into places model
renatoo created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY Added baloo dependency for filewidgets project Added new sections for places model: 'Recently Saved' and 'Search for' Based on dolphin code TEST PLAN From an kde app try to open/save a file. It should show a new file dialog with new categories on the places panel REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 AFFECTED FILES CMakeLists.txt autotests/kfileplacesmodeltest.cpp src/filewidgets/CMakeLists.txt src/filewidgets/kfileplacesmodel.cpp src/filewidgets/kfileplacesview.cpp To: renatoo Cc: #frameworks