Expose group information to Dolphin's Information panel, tooltips, etc
ClosedPublic

Authored by ngraham on Apr 3 2018, 3:02 AM.

Details

Summary

Expose Group information for files and folders so that it can be shown in Dolphin's Information panel, tooltips, etc.

FEATURE: 308002
FIXED-IN: 18.08.0

Test Plan

Deploy change, create new user, open Dolphin, open Information Panel

  • Group is not visible by default, as expected
  • When made visible, Group appears between User and Permissions, as expected

Deploy change, use existing user account, open Dolphin, open Information Panel

  • Group is not visible by default, as expected
  • When made visible, Group appears between User and Permissions, as expected

Diff Detail

Repository
R824 Baloo Widgets
Branch
expose-group-information (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
ngraham created this revision.Apr 3 2018, 3:02 AM
Restricted Application added a project: Baloo. · View Herald TranscriptApr 3 2018, 3:02 AM
ngraham requested review of this revision.Apr 3 2018, 3:02 AM
ngraham edited the test plan for this revision. (Show Details)Apr 3 2018, 3:05 AM
ngraham edited the summary of this revision. (Show Details)
ngraham added a subscriber: Dolphin.

This is optional and off-by-default, right? +1 then

+1 Let's wait a little for more opinions.

src/metadatafilter.cpp
115

Because of default == true, Group shows up for "old users" as kfileitem#group is not defined for them.
settings.readEntry(uriString, false) would mitigate that, but then the config dialog of infopanel is out of sync. I. e. showing a checked Group.
I think we should leave it as is. Group hiding is only 4 clicks away for "old" == experienced users.

Yes, this is optional-and-off-by-default for new user accounts and for existing users who have not customized the Information Panel before.

Michael brings up a good point though: this will cause existing users who have customized what's displayed in the information panel to get the new entry automatically. But maybe we can live with that and such users are sufficiently technically adept that they'll either like it or be able to easily turn it off?

We might also want to come up with a generic solution for that issue, or else we'll hit it over and over again whenever we add a new piece of data that the Information Panel can display.

michaelh added a comment.EditedApr 3 2018, 4:24 PM

We might also want to come up with a generic solution for that issue, or else we'll hit it over and over again whenever we add a new piece of data that the Information Panel can display.

In that case I would go for an generally opt-in solution with a few options checked by default. Just enough to convey whats possible. To be specific:
Showing up for any file:

  • Tags
  • Comment
  • Rating
  • Type or Date
  • Size

Additionally:

  • Title (covers all media: audio, video, ebooks and Documents)
  • Width & Height (covers all images and video)

With this users will experience that info changes with the file type and hopefully get curious to discover more.

We might also want to come up with a generic solution for that issue, or else we'll hit it over and over again whenever we add a new piece of data that the Information Panel can display.

In that case I would go for an generally opt-in solution with a few options checked by default. Just enough to convey whats possible. To be specific:
Showing up for any file:

  • Tags
  • Comment
  • Rating
  • Type or Date
  • Size

    Additionally:
  • Title (covers all media: audio, video, ebooks)
  • Width & Height (covers all images and video)

In fact, this is exactly what we currently have by default in 18.04! :-)

What I'm saying for this is that we need a generic method to add additional data that can be displayed using the Information Panel without making those data displayed by default for users who have previously customized what's displayed in their Information Panels.

zzag added a subscriber: zzag.EditedApr 3 2018, 6:37 PM

Could you please also expose file permissions? (I ask as a user)

That's already available for those who want it. Right-click in the Information Panel, choose Configure... and add whatever you want.

michaelh added a comment.EditedApr 3 2018, 6:43 PM

In fact, this is exactly what we currently have by default in 18.04! :-)

Let's go for a drink then.

What I'm saying for this is that we need a generic method to add additional data that can be displayed using the Information Panel without making those data displayed by default for users who have previously customized what's displayed in their Information Panels.

I understood. What I meant by opt-in was settings.readEntry(uriString, false) which would show nothing except when explicitely set to true and true by default would be the above mentioned list.

zzag added a comment.EditedApr 3 2018, 6:44 PM

That's already available for those who want it. Right-click in the Information Panel, choose Configure... and add whatever you want.

Thanks! I didn't know that the Information panel is configurable. :D

In fact, this is exactly what we currently have by default in 18.04! :-)

Let's go for a drink then.

What I'm saying for this is that we need a generic method to add additional data that can be displayed using the Information Panel without making those data displayed by default for users who have previously customized what's displayed in their Information Panels.

I understood. What I meant by opt-in was settings.readEntry(uriString, false) which would show nothing except when explicitely set to true and true by default would be the above mentioned list.

Ah, I understand now. Yes, opt-in rather than opt-out makes sense to me. I'll experiment with that later.

Thanks! I didn't know that I can configure the Information panel. :D

@ngraham Mr. usability, your comment...?

Thanks! I didn't know that I can configure the Information panel. :D

@ngraham Mr. usability, your comment...?

Hah, I was in fact already having those thoughts...

It would probably make sense to move these settings into the Configure window, where all the other Dolphin settings live. It's always been a bit odd that the only place to configure the Information Panel was hidden away in a context menu.

It would probably make sense to move these settings into the Configure window, where all the other Dolphin settings live. It's always been a bit odd that the only place to configure the Information Panel was hidden away in a context menu.

Which reminds me of my last comment in D11245

zzag added a comment.Apr 3 2018, 6:52 PM

Could you please enable file permissions by default? File permissions are way useful than Tags or Rating, which are already enabled by default.

In D11897#239271, @zzag wrote:

Could you please enable file permissions by default? File permissions are way useful than Tags or Rating, which are already enabled by default.

The people for whom file permission information is useful (technically oriented sysadmin/IT/programmer types who administer or work in a multi-user environment) should be able to find out for themselves how to enable this--or will once we make it more obvious! :-) I'm not sure that file permission information is relevant for most of Dolphin's target users, especially considering that the output right now is not human-readable (e.g. "rw-r--r--").

Yep, my guess is most users rule their system anyway. For me they provide no useful info as I never navigate into say /sys/bus/clockevents/ with dolpin. There's a konsole for those things.

zzag added a comment.Apr 3 2018, 7:18 PM
In D11897#239271, @zzag wrote:

Could you please enable file permissions by default? File permissions are way useful than Tags or Rating, which are already enabled by default.

The people for whom file permission information is useful (technically oriented sysadmin/IT/programmer types who administer or work in a multi-user environment) should be able to find out for themselves how to enable this--or will once we make it more obvious! :-) I'm not sure that file permission information is relevant for most of Dolphin's target users, especially considering that the output right now is not human-readable (e.g. "rw-r--r--").

Yep, my guess is most users rule their system anyway. For me they provide no useful info as I never navigate into say /sys/bus/clockevents/ with dolpin. There's a konsole for those things.

Well, I often wonder whether a given file is an executable or writable..


Now, for example, I see that blur-lockscreen.sh is in fact an executable.

now is not human-readable (e.g. "rw-r--r--").

Are there alternatives to traditional Unix permissions?
Personally, I think it's okay to show traditional Unix file permissions because Linux is a Unix-like operating system* but I'm a "developer" so maybe my opinion shouldn't count.

* Yes, yes, I know... Linux is a kernel.

In D11897#239275, @zzag wrote:

Well, I often wonder whether a given file is an executable or writable..

To me, this suggests that we might want to badge the icon itself or something, because this is generally useful information, and also has security implications (you don't want executable programs masquerading unseen as regular files).

In D11897#239275, @zzag wrote:

Personally, I think it's okay to show traditional Unix file permissions because Linux is a Unix-like operating system* but I'm a "developer" so maybe my opinion shouldn't count.

Heh, it's not that your opinion doesn't count, but rather that we trust skilled people with specialized needs to optimize their own workspaces as needed. We optimize general-purpose tools for the common case, not the specialized case.

"Simple by default... yadda yadda yadda". In this case, it seems that the "powerful when needed" part of customizing the Information Panel's display was too well hidden, and we can and should improve that (in another patch, obviously).

src/metadatafilter.cpp
115

but then the config dialog of infopanel is out of sync

Nonsense!
What you need is here:
https://cgit.kde.org/baloo-widgets.git/tree/src/filemetadataconfigwidget.cpp#n119

ngraham added inline comments.Apr 3 2018, 8:16 PM
src/metadatafilter.cpp
115

Sounds good! Would you like to submit a patch for this?

$ kde/applications/baloo-widgets (string-list-widget)>git branch --list | wc -l
28

Do I have to?

$ kde/applications/baloo-widgets (string-list-widget)>git branch --list | wc -l
28

Do I have to?

Hah, of course not! I was just thinking that it might be a trivial 5-minute patch. If not, I can take a look myself when I find the time.

Ok, ok,..

  • Tags
  • Comment
  • Rating
  • Type or Date
  • Size

    Additionally:
  • Title (covers all media: audio, video, ebooks)
  • Width & Height (covers all images and video)

In fact, this is exactly what we currently have by default in 18.04! :-)

Type or Date?

Type or Date?

Both: Type and Date Modified.

Not trivial: Tests crash.

Patch does not contains strings why it's targeted for 18.08?

I was just being conservative; we can land in 18.04 if you'd prefer, assuming it's accepted in time.

cfeck added a subscriber: cfeck.Apr 3 2018, 9:42 PM

Patch does not contains strings why it's targeted for 18.08?

I18N_NOOP2_NOSTRIP("@label", "Group") is a new string.

The real question is, why Baloo Widgets is not part of Frameworks :P

Ah right, so it is. And yeah, it is weird that this project isn't on the Frameworks release schedule.

ngraham updated this revision to Diff 31245.Apr 3 2018, 10:50 PM

Update the version, which fixes the issue of existing users getting the new data

ngraham edited the test plan for this revision. (Show Details)Apr 3 2018, 10:54 PM
ngraham marked 3 inline comments as done.

Forgot that there actually already is a method to ensure that existing users don't get the change: increase the version number of the file. Now the upgrade case works just fine. This is ready for review.

Just for the fun of it. You might want to this to this stack: D11913.

michaelh added a comment.EditedApr 4 2018, 10:18 AM

Forgot that there actually already is a method to ensure that existing users don't get the change: increase the version number of the file. Now the upgrade case works just fine. This is ready for review.

Arrgh: Is D11913 dispensable now? It does make clearer what the defaults are though.

ngraham added a comment.EditedApr 4 2018, 2:07 PM

Forgot that there actually already is a method to ensure that existing users don't get the change: increase the version number of the file. Now the upgrade case works just fine. This is ready for review.

Arrgh: Is D11913 dispensable now? It does make clearer what the defaults are though.

I think it's quite worthwhile to use a whitelist instead of a blacklist. But that's now a general enhancement and not related to this patch. I'll review it.

michaelh accepted this revision as: michaelh.Apr 4 2018, 4:10 PM
This revision is now accepted and ready to land.Apr 4 2018, 4:10 PM
This revision was automatically updated to reflect the committed changes.
bruns added a subscriber: bruns.Apr 4 2018, 11:16 PM
In D11897#239275, @zzag wrote:

now is not human-readable (e.g. "rw-r--r--").

Are there alternatives to traditional Unix permissions?
Personally, I think it's okay to show traditional Unix file permissions because Linux is a Unix-like operating system* but I'm a "developer" so maybe my opinion shouldn't count.

There are ACLs, which are just shown as "rw-r--r--+" ...

bruns added a comment.Apr 4 2018, 11:24 PM
In D11897#239275, @zzag wrote:

Well, I often wonder whether a given file is an executable or writable..

To me, this suggests that we might want to badge the icon itself or something, because this is generally useful information, and also has security implications (you don't want executable programs masquerading unseen as regular files).

At least for "writable", there is already the lock icon badge.

In D11897#239275, @zzag wrote:

Personally, I think it's okay to show traditional Unix file permissions because Linux is a Unix-like operating system* but I'm a "developer" so maybe my opinion shouldn't count.

Heh, it's not that your opinion doesn't count, but rather that we trust skilled people with specialized needs to optimize their own workspaces as needed. We optimize general-purpose tools for the common case, not the specialized case.

"Simple by default... yadda yadda yadda". In this case, it seems that the "powerful when needed" part of customizing the Information Panel's display was too well hidden, and we can and should improve that (in another patch, obviously).

Show an additional "screw driver" icon after "Unlock panel" in the context menu has been selected?
Add a "Sidebar" category in the dolphin configuration, or add it as a new tag under "General"? (My preference: new Category).

Any chance this change broke the unit test filemetadataitemcounttest, due to new items?
See https://build.kde.org/view/Applications/job/Applications%20baloo-widgets%20kf5-qt5%20SUSEQt5.9/34/

Please have a look if you can make CI happy again :)

Oooh darn, for some reason I've missed these test failures in baloo-widgets, sorry! Will fix.

After investigating, it looks like this failure may be an installation issue on the build machine. The test is failing there because the new group property is inappropriately visible, but the approved change expressly covered this case by adding it to the baloofileinformationrc blacklist and bumping the version, which should have had the effect of not making the property visible. I suspect baloofileinformationrc did not get updated on the build machine the way it did on my local machine, where the test passes.

@bcooksley, any chance you can take a look at this?

After investigating, it looks like this failure may be an installation issue on the build machine. The test is failing there because the new group property is inappropriately visible, but the approved change expressly covered this case by adding it to the baloofileinformationrc blacklist and bumping the version, which should have had the effect of not making the property visible. I suspect baloofileinformationrc did not get updated on the build machine the way it did on my local machine, where the test passes.

@bcooksley, any chance you can take a look at this?

@ngraham, @bcooksley : de-warning, my bad. The test I linked to by some magic had started to no longer fail in https://build.kde.org/view/Applications/job/Applications%20baloo-widgets%20kf5-qt5%20SUSEQt5.9/38/ (scripty surely did not fix it, or? :) there might be something dirty still, but we have to wait for it to return if there is), but was replaced by another test failing. I just looked at the graph and assumed it was the same test failing, given the constant number 1.

That test FileMetadataItemCountTest does not fail for me locally as well with latest master of KF & Qt 5.11.1.

(And the new test failing, extractortest, should hopefully be fixed soon as well, as sideeffect of the regression fix patch D13885)

In regards to CI nodes, for Linux virtually nothing is retained between builds, with the only writeable path that is kept being the directory where the archive cache is kept (and that is at a path unique to the CI system which nobody else should be touching)