Show list of tags in PlacesView
ClosedPublic

Authored by nicolasfella on Sep 5 2017, 8:08 PM.

Details

Summary

Show list of available Tags in PlacesView. In addition show "All tags" entry.
BUG: 182367

Test Plan

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes
markg added a comment.Sep 23 2017, 4:27 PM

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.

markg added a comment.Sep 23 2017, 4:30 PM

If we can make it a separate panel and overcome some of the disadvantages of having multiple panels. I'm fine with it. That means:

  1. Make the Tags panel visible by default
  2. Include a visible separator between panels (https://bugs.kde.org/show_bug.cgi?id=384999)

    I can work on https://bugs.kde.org/show_bug.cgi?id=384999.

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..

cfeck added a comment.Sep 23 2017, 4:33 PM

Ideally, the title does not scroll away at all.

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.

markg added a comment.Sep 23 2017, 4:45 PM

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 planned changes to this revision.Sep 24 2017, 10:05 AM
ngraham added a comment.EditedOct 23 2017, 3:42 PM

@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.

Agreed, that makes sense. Then it would show up in file dialogs too.

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?

@alexeymin

how do I force a reindex?

Just in case...

$ balooctl clear <file>
$ balooctl index <file>

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?

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).

What do you think would be a reasonable number of tags? 5, 10, 20, ..? Make it configurable?

Please avoid configurable options as much as possible ;)

It seems that there is already a consensus about a reasonable number of tags -> so 8 ;)

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?)

michaelh added a comment.EditedJan 30 2018, 1:22 PM

Tried <F5>?

Tried <F5>?

Yes. No change. And tried on different devices.

@nicolasfella, what are the next steps here?

markg removed a subscriber: markg.Jan 30 2018, 7:33 PM

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.

markg added a comment.EditedJan 30 2018, 7:59 PM

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?

mmustac added a subscriber: mmustac.EditedJan 31 2018, 1:52 PM

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.

rkflx added a subscriber: rkflx.Jan 31 2018, 2:06 PM

I could not try this one. arc patch D7700 failed against master and Applications/17.12.

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.

I'll look into it the next days

@nicolasfella Beware tags:/ currently is a little broken.

$ kioclient5 ls "tags:/Watched"
Log Horizon?/home/otto/Videos/Log Horizon

Citing myself

Just as an idea: Implement user defineable groups. That way users could drop their most important tags/searches/whatever into it and expand and collapse as they wish.

bruns added a subscriber: bruns.Apr 6 2018, 6:47 PM

Add tags places in KIO

Restricted Application added a project: Frameworks. · View Herald TranscriptJul 8 2018, 12:42 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript

Actually save tag bookmark and check if it already exists before adding

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.

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

Me neither. Can you rebase this on current master?

  • Merge branch 'master' into arcpatch-D7700
ngraham accepted this revision.Nov 13 2018, 2:35 PM

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.

This revision is now accepted and ready to land.Nov 13 2018, 2:35 PM
nicolasfella added inline comments.Nov 13 2018, 5:05 PM
src/filewidgets/kfileplacesmodel.cpp
187

Will fix on push

This revision was automatically updated to reflect the committed changes.
src/filewidgets/kfileplacesmodel.h
66

This is missing a ///< @since 5.53 comment.

nicolasfella reopened this revision.Dec 27 2018, 12:57 PM
This revision is now accepted and ready to land.Dec 27 2018, 12:57 PM

Tags List v2.0

Deleted tags are automatically removed from the panel

nicolasfella requested review of this revision.Dec 27 2018, 12:58 PM
  • Whitespace--
ngraham added a subscriber: broulik.

@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.

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

Can you rebase this on current master? I'll give it another shot and report back.

ngraham accepted this revision.Mar 19 2019, 11:19 AM

Yep, works for me too now.

This revision is now accepted and ready to land.Mar 19 2019, 11:19 AM
This revision was automatically updated to reflect the committed changes.
chehrlic added inline comments.
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.

kossebau added inline comments.
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?

nicolasfella added inline comments.Nov 25 2021, 7:46 PM
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