Added new sections for places model: 'Recently Saved' and 'Search for' with baloo urls
Based on dolphin code
Depends on D8243
Details
Details
- Reviewers
dvratil ngraham ervin mlaurent dfaure mwolff - Group Reviewers
Frameworks Dolphin KDE Applications VDG - Commits
- R241:7eb6333bdb48: Added baloo urls into places model
From an kde app try to open/save a file.
It should show a new file dialog with new categories on the places panel
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Lint
Lint Skipped - Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
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 | ||
|---|---|---|
| 98 ↗ | (On Diff #20905) | 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 | ||
| 68 ↗ | (On Diff #20905) | 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. |
| 499 ↗ | (On Diff #20905) | 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 | ||
| 1299 ↗ | (On Diff #20905) | Don't you want to match /yesterday and so on here? Like you're matching /documents and so on for the method below. |
| 1327 ↗ | (On Diff #20905) | 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 | ||
|---|---|---|
| 68 ↗ | (On Diff #20905) | 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? |
| autotests/kfileplacesviewtest.cpp | ||
|---|---|---|
| 2 ↗ | (On Diff #21647) | ? I don't think that you are kevin ottens :) and we are in 2017 :) |
| 20 ↗ | (On Diff #21647) | QtCore/ is not necessary |
| 30 ↗ | (On Diff #21647) | QtTest/ not necessart too |
| 70 ↗ | (On Diff #21647) | new line before it |
| 111 ↗ | (On Diff #21647) | use new connect api for QSignalSpy |
| src/filewidgets/kfileplacesview.cpp | ||
|---|---|---|
| 1346 ↗ | (On Diff #20905) | won't be nice to add an else statement? (for the sake of searchUrl ) |
| autotests/kfileplacesviewtest.cpp | ||
|---|---|---|
| 30 ↗ | (On Diff #21807) | 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 | ||
|---|---|---|
| 153 | Minor: QLatin1String("true") is enough for a comparison (no need to use a 16-bit-per-char string) | |
| src/filewidgets/kfileplacesview.cpp | ||
| 1348 ↗ | (On Diff #20905) | 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. |
Comment Actions
lgtm, one minor nit, potentially for the future
| src/filewidgets/kfileplacesmodel.cpp | ||
|---|---|---|
| 967 ↗ | (On Diff #20905) | this could/should be a free function, not a member, considering its result is cached in a member variable |
| autotests/kfileplacesviewtest.cpp | ||
|---|---|---|
| 70 ↗ | (On Diff #22775) | 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 | ||
|---|---|---|
| 68 ↗ | (On Diff #20905) | const is fine then. |
Comment Actions
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 KDevelop, #Okteta, Kile, Kate and 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.
Comment Actions
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.
Comment Actions
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?
Comment Actions
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…)
Comment Actions
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?