Show list of available Tags in PlacesView. In addition show "All tags" entry.
BUG: 182367
Details
- Reviewers
alexeymin ngraham broulik - Group Reviewers
Dolphin KDE Applications - Maniphest Tasks
- T8349: Improve Places panel usability and presentation
- Commits
- R241:2ae8a0c374cd: Show list of tags in PlacesView
R241:523acf53dadc: Show list of tags in PlacesView
Assign some tags to files, force a reindex of the files and restart Dolphin to see the result
Diff Detail
- Repository
- R241 KIO
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Again, same with recentdocuments, if stuff like this is added by default, it needs to work just fine in the normal usecases, otherwise it will cause bug reports which will in term cause the tags part to be disabled by default. Yes, chicken and egg problem plays here as well.
I'm working on improving the tags ioslave. D7855 was a first step on this.
Nice! :)
Just curious, i have some files with tags (just added them). Baloo is running, but tags:/ is empty...
System monitor shows a baloo_file running at 100% cpu...I'm not an expert on baloo, but waiting and klicking Refresh in dolphin should make it work
Ahh, that indeed seems to be the case. Lots of waiting that is.
Memory usage also shoots up when baloo_file and baloo_file_extractor are running (those two combined take up ~3.5GB of memory here at the moment).
Tag changes don't seem to immediately be propagated to tags:/ .. Far from it. More like half an hour or so, hehe.
That would be my preference indeed. Be sure to check with Emmanuel for this as well.
As for No. 2; That will get you into style issues i think. And when no scrollbar is visible, you probably don't want to show the separator as there likely is a title visible anyhow. Perhaps if the title disappears show a separator line..
The title is only visible when the panels are unlocked. But in fact, the problem goes away when panels are unlocked; it's only a problem when locked. I'm working on a patch that improves this when panels are locked. Either way, we should continue that discussion in https://bugs.kde.org/show_bug.cgi?id=384999.
I vaguely remember that being an issue in the Places panel as well from a couple years ago (no title when locked). I think the title was then added to the places panel. That now does give you the odd double "Places" when the panels aren't locked.
Imho, it would be fine if you add a title in your new panel just like places has one. So in unlocked mode your "Tags" panel will have "Tags" two times below one another, just like "Places" :) Lets call it consistency, hehe.
Seems like a simple solution is to always show the title. That way, it wouldn't scroll away and you would still see it when locked. We would have to remove the redundant title from the Places panel with this, but that's simple enough.
I've submitted a patch to Breeze that resolves the usability issues with having multiple adjacent DockWidget panels: https://phabricator.kde.org/D7957
Merging that (or otherwise fixing the issue) will eliminate my objections to putting this in a Panel that's visible by default.
@nicolasfella, where are we at with this one? @markg makes a lot of good points that I ultimately agree with, but for now I think it does still make sense to put Tags in the Places panel unless and until we get the proposed new design of making everything a separate panel and putting all panels into a single scrollview. Until that happens (and I do support it), let's just use the Places panel for now.
IMHO we can merge it for now (maybe I will have to rebase) I like the idea of separate panels and I'm working on something similar. I try to extract generic pieces from the PlacesPanel to an abstract ListPanel that can be extended to create all kinds of panels we want. I'm not done yet but I hope I can show you something soon
I'd suggest to wait for the effort of upstreaming the places model in KIO (see @renatoo 's revisions), before merging this.
I must have misunderstood something in this discussion. The functionality is there already.
Activate folderpanel when navigating to tags:// you get.
Why an extra panel?
how do I force a reindex?
Just in case...
$ balooctl clear <file> $ balooctl index <file>
Because of usability.
Majority of users wouldn't know that.
It adds extra work.
It's more efficient to see the tags (if you're using them) consistently, without extra work, to drag to and from them etc... (if you don't use them, you close the panel).
I disagree. In my organisation we use tags, instead of folders. We only want to see the tags panel. We don't want to have to click 'see more' each time.
Adding a discrete configuration option in loses nothing and gains everything for those who regularly use tags as a means of semantic organisation (better than folders).
Please at least have a configurable number shown option (including show all).
We don't restrict the number of folders shown in the other panels!
It should be noted that Baloo seems to be very slow in updating tags to the index. So users will not see their tagged items appear under the relevant tag for some time after tagging.
Trying to file a bug, but seeing if anyone knows about this already first (may be an indexing issue?)
The patch can't stay as-is, because the model was replaced by the one from KIO. We could either include this patch into KIO (which would raise concerns about having too much stuff there) or make it a separate panel. I once planned to extract common stuff from places panel and tags panel to a common ancestor, but that's quite some work. I'll try to put together a standalone Tags Panel soon
FWIW I'm still in favor of putting this in the Places panel via KIO, especially now that we have support for showing and hiding categories. If we make this its own new panel, I'd want to push for adding it to the default layout in Dolphin, or else nobody will ever see it and the work will be wasted.
And that very reasoning is the exact reason why i was against the "cram everything in the places panel" movement.
But that's done now, and for good reasons. I know.
But this does kinda kill the multiple panel idea in dolphin and effectively makes the Places panel "the" panel.
Having said that, i don't care enough to fix it. I haven't provided patches (like you folks have) nor do i have the time for it anyhow.
I'm fine with it either way.
Note that i do very much like the results it brings!
Note 2: Why do i still get mails from this thread when i unsubscribed.. Weird phabricator...
edit
Just wanted to add that i'm fine if the tags get added to the places panel. If people want that, so be it. However, I will object to having them shown by default! The tags are still thoroughly slow and sluggish that it would never give a good user experience as-is. If that's fixed, be my guest and show it by default as well.
For all of this, no +1 or -1 from me. I'm just voicing my opinion with arguments.
Thanks for your perspective, Mark. I appreciate your point of view.
@nicolasfella, sounds like you have the green light to adapt this patch to apply it to KIO, and we'll see how that turns out! I for one am really looking forward to it.
I could not try this one. arc patch D7700 failed against master and Applications/17.12.
This looks like a raw diff. If so, please consider using arc.
@ngraham: Are special techniques involved to arc patch a raw diff?
@spoorun: Probably Bug 388481. Mind to add your observations to it?
I know this has nothing to do with this patch - maybe I should file a bug for this ? - anyway I wanted to share my thoughts after reading Mark's position and I think he's right. We should not put everything into the places panel nor should it be a sperate panel ... sound weird ? Well ... I like the idea of having different panels for different working scenarios but the static job they were created for is not ideal, altough we can hide groups from the panel now. My idea is to be able to add one ore more panels which would be just a container which then can be filled with different views / models ? (Like toolbars) So for example I can create a panel by my own, give it a name and customize it to show a list of tags and shortcuts for my saved searches for example. Then another panel could contain my device list and the default places section and a third one to contain the directory tree. That way I would be able to switch very quick between them, don't need to scroll or reconfigure my panel over and over again. Maybe someone can follow me and what I am trying to describe. Dolphin then could offer some preconfigured panels by default but let the users all possibilities to customize it wahtever they like. Easy by default, powerfull when needed.
I know this isn't the most technically elegant solution, but bikeshedding has already cost this patch 4 months and it now requires a total rebase on top of KIO. The more hurdles we put in Nico's path, the less likely it is to ever get implement in any form unless someone else wants to take it over.
That's an interesting idea, @mmustac, but as you said, it's somewhat unrelated to this particular patch. Maybe file a bugzilla wishlist ticket for it?
@michaelheidelbach, applying the diff fails because the functionality this was built on top of has since been moved to KIO (which was really welcome work). This patch will need to be re-based (or more likely) re-written on top of KIO.
Try against Applications/17.08 or any commit close to the upload date. Nevertheless, @nicolasfella should rebase this patch on current master eventually.
This looks like a raw diff. If so, please consider using arc.
Phab shows a popup if you point your mouse over the date/time entry of a comment indicating a patch upload or update. Here we see "Via Web", but obviously arc would be more friendly for reviewers because of the included base revision.
Regarding the topic: I did not read everything in detail, but please make sure not to break Gwenview and other users of KFilePlaces again, I'm still busy with the fallout from the last breakage… Also, performance matters, so ensure (and measure!) we won't regress for Dolphin's default config.
@nicolasfella, could we resurrect this and re-base it on top of KIO? It's really great functionality, and I'd hate to see it lost to the sands of time.
@nicolasfella Beware tags:/ currently is a little broken.
$ kioclient5 ls "tags:/Watched" Log Horizon?/home/otto/Videos/Log Horizon
Results of testing:
- After I create a new tag, it doesn't show up in the Places panel tag list or under the tags:/ ioslave until I restart Dolphin.
- When I navigate to tags:/ ioslave using the All Tags item in the Places panel and I click on a tag, its icon disappears.
- When I remove a tag from an item and navigate to that tag entry on the Places panel, the un-tagged item still shows up there until I restart Dolphin.
- When I add a tag to an item and navigate to that tag entry on the Places panel, the item does not show up there until I restart Dolphin.
Some of these issues may be issues with the tags:/ ioslave or in Baloo, but I think they need to be fixed before we push this feature front-and-center, or else we'll get a ton of bug reports.
I can't reproduce these issues, so I think that the underlying Baloo issues have been solved.
I suggest to reconsider this patch for inclusion, especially since it's non-invasive for users that don't make use of tags at all. If there are issues remaining it's easier to spot them when people are actually using it
A surprisingly small amount of code for a nice feature. Code looks good to me except for one minor thing. This works great in my testing now. I say let's ship it, especially because it won't result in any UI changes at all for people who don't use tags.
src/filewidgets/kfileplacesmodel.cpp | ||
---|---|---|
187 | const. But in fact, does this even need to be a variable? It's only used once. |
src/filewidgets/kfileplacesmodel.cpp | ||
---|---|---|
187 | Will fix on push |
src/filewidgets/kfileplacesmodel.h | ||
---|---|---|
66 | This is missing a ///< @since 5.53 comment. |
@broulik I believe you were the one who found a bunch of issues last time; can you re-test? I can't remember what your concerns were, sorry!
Bugs found:
- If there are no tags, creating one does not cause the Tags section to appear in the Places panel until Dolphin is restarted.
- If the Tags section is already visible because there are already some tags, creating a new tag does not cause that new tag to appear in the Tags section until Dolphin is restarted.
Those work fine for me. Anyway, those appear to be problems with Baloo which are probably resolved by now
src/filewidgets/kfileplacesmodel.cpp | ||
---|---|---|
172 | While I way playing around with asan, I got a report that tagsLister is leaking. I can't see a deletion of this object anywhere, even in current code. |
src/filewidgets/kfileplacesitem.cpp | ||
---|---|---|
351 | @nicolasfella Hi. this is passing the tag string both as translation context as well as the string to translate as well. Is this on purpose? Even more as the context parameter is ignored anyway and just there to force a context for the other cases being used with I18NC_NOOP values, to make sure a context is set at all. So where are those tag names coming from, are they expected to be translated at all? |
src/filewidgets/kfileplacesitem.cpp | ||
---|---|---|
351 | Tags are user-defined string and not supposed to be translated at all. So yeah, passing the tag as a context makes no sense here. Most likely I didn't even understand the concept of a translation context at the time of writing |