Details
- Reviewers
markg ngraham michaelh - Group Reviewers
Dolphin - Commits
- R318:7d463ddd04bf: infopanel: Add choice of date display formats
visual inspection
Diff Detail
- Repository
- R318 Dolphin
- Branch
- arcpatch-D11245
- Lint
No Linters Available - Unit
No Unit Test Coverage
Why directly in the RMB menu?
It's not like users will often do that (i think).
Is there a reason for it not to be in the settings for that panel?
Yes, it took me some time to discover it.
Is there a reason for it not to be in the settings for that panel?
Its just my personal perference. Some reasons:
I prefer the config dialog to only contain the list of selectable properties.
The context menu is still short.
Dolphin's main config dialog is already confusingly crowded.
Only 2 clicks away.
and it had to go somewhere for a start ;-)
RMB = ? Right mouse button ?
I think having it in the context menu makes enough sense; the toggle for the preview is already there, so there's already precedent for having the Information Panel's UI be controlled via context menu. +1.
src/panels/information/informationpanelcontent.cpp | ||
---|---|---|
287 | Not sure this is the right icon. How about change-date-symbolic? |
@broulik and I seem to think that it should be in the settings, not under the RMB.
To be fair, i'm trusting your judgement so i'm fine either way, but i'm "leaning" towards in the settings as well.
src/panels/information/informationpanelcontent.cpp | ||
---|---|---|
120–121 | No c-style cast. | |
197 | No c-style cast. | |
289 | No c-style cast. | |
307–310 | Err, auto for int.. No, just int! Don't misuse auto. |
If we go down that path, I would argue that it makes sense to relocate the [ ] preview checkbox that's currently in the context menu to that window as well for consistency's sake. I'd be fine with it as long as we do that to maintain consistency.
There's more to consider. With D10803 coming up tooltips may be overwhelmed with info when they continue to follow the settings in baloofileinformationrc. In the long run we'll probably need separate settings for tooltips and infopanel. baloo-widgets has to offer that option of course. (I'm willing to adapt it)
In conclusion: I also prefer using dolphin's settings.
But why not also keep it here as a shortcut?
Irrelevant for this change.
In conclusion: I also prefer using dolphin's settings.
But why not also keep it here as a shortcut?
And why not keep the [ ] Preview? And why not add another handy button that translates your image metadata coordinates to an embedded google maps at the flick of a small switch under the RMB?
You see where i go here?
You could keep on adding functions to the RMB which really are just settings.
The RMB should imho be reserved for opening the settings panel and executing actions on whatever you pressed RMB under. Changing how it looks is not executing an action on it.
You could argue (and i'd probably agree with that) that the settings panel itself is a poor choice and having a hamburger or gear icon appear when you hover the panel which opens the settings would be more intuitive.
I'd like to see the Information Panel's configuration moved into Dolphin's Configure window. But IMHO we shouldn't block a good improvement on that. For now, I think it's okay to land this as-is, provided Dolphin maintainers agree.
The list of properties presented in config dialog changes with the currently selected file, showing only those applicable. This cannot (and should not) be done in Dolphin's Configure window, I think.
In Dolphin's Configure window property selection should be structured somehow, see https://api.kde.org/frameworks/baloo/html/searching.html. That's beyond the scope of this patch, imo.
Personally, I just want the short date display. For me it's OK to remove the context menu item and for the time being manually change the setting in dolphinrc.
Hmm, that's true.
In Dolphin's Configure window property selection should be structured somehow, see https://api.kde.org/frameworks/baloo/html/searching.html. That's beyond the scope of this patch, imo.
Yes, definitely.
@markg, do you think this can go in as-is, and we can work on redoing the configuration UI elsewhere?
The current baloo-widgets version 5.0.0 (build/baloowidgets_version.h) has not been changed since April 2014 (https://cgit.kde.org/baloo-widgets.git/diff/CMakeLists.txt?id=a62a6a223b7927bcc18c625f436e454b6601255e). Shouldn't that be bumped first?
Contrary to baloo it seems not to be bumped automatically.
Right, we should definitively do that first.
In order to have it bumped automatically we'd need some cmake magic, see https://community.kde.org/Guidelines_and_HOWTOs/Application_Versioning