Added new sections for places model: 'Recently Saved' and 'Search for' with baloo urls
Based on dolphin code
Depends on D8243
dvratil | |
ngraham | |
ervin | |
mlaurent | |
dfaure | |
mwolff |
Frameworks | |
Dolphin | |
KDE Applications | |
VDG |
Added new sections for places model: 'Recently Saved' and 'Search for' with baloo urls
Based on dolphin code
Depends on D8243
From an kde app try to open/save a file.
It should show a new file dialog with new categories on the places panel
Lint Skipped |
Unit Tests Skipped |
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.
autotests/kfileplacesmodeltest.cpp | ||
---|---|---|
99 | 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. | |
src/filewidgets/kfileplacesmodel.cpp | ||
77 | 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. | |
507 | 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. | |
src/filewidgets/kfileplacesview.cpp | ||
1301 | Don't you want to match /yesterday and so on here? Like you're matching /documents and so on for the method below. | |
1329 | 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. |
src/filewidgets/kfileplacesmodel.cpp | ||
---|---|---|
77 | 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? |
src/filewidgets/kfileplacesview.cpp | ||
---|---|---|
1348 | won't be nice to add an else statement? (for the sake of searchUrl ) |
autotests/kfileplacesviewtest.cpp | ||
---|---|---|
31 | Please don't include the full module (which also contains all QtCore) #include <QTest> is OK, #include <QtTest> is not. Confusing, I know. |
src/filewidgets/kfileplacesmodel.cpp | ||
---|---|---|
161 | Minor: QLatin1String("true") is enough for a comparison (no need to use a 16-bit-per-char string) | |
src/filewidgets/kfileplacesview.cpp | ||
1350 | 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. |
lgtm, one minor nit, potentially for the future
src/filewidgets/kfileplacesmodel.cpp | ||
---|---|---|
967 ↗ | (On Diff #22328) | this could/should be a free function, not a member, considering its result is cached in a member variable |
autotests/kfileplacesviewtest.cpp | ||
---|---|---|
71 ↗ | (On Diff #22328) | typo: Contains. I would just have called it testUrlChanged because it also checks that it's emitted, not just what the emitted value is :) |
src/filewidgets/kfileplacesmodel.cpp | ||
---|---|---|
77 | const is fine then. |
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:
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:
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.
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?
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…)
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?