infopanel: Add choice of date display formats
AbandonedPublic

Authored by elvisangelaccio on Mar 11 2018, 6:52 PM.

Details

Summary

Let user choose date display format via checkbox in context menu

Example

Depends on D11242
Depends on D12342

FEATURE: 392352

Test Plan

visual inspection

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D11245
Lint
No Linters Available
Unit
No Unit Test Coverage
michaelh requested review of this revision.Mar 11 2018, 6:52 PM
michaelh created this revision.
michaelh edited the summary of this revision. (Show Details)
markg added a subscriber: markg.Mar 12 2018, 11:21 AM

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?

It's not like users will often do that (i think).

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.

ngraham added inline comments.Mar 13 2018, 4:29 AM
src/panels/information/informationpanelcontent.cpp
287

Not sure this is the right icon. How about change-date-symbolic?

michaelh updated this revision to Diff 29379.Mar 13 2018, 8:02 AM
  • infopanel: Change icon

+1, I like it!

Shouldn't that rather be in the "Configure" dialog?

markg requested changes to this revision.Mar 14 2018, 10:18 PM

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.

@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.
Also, put this on one line. Nobody formats a cast like that and it imho makes it less readable.

This revision now requires changes to proceed.Mar 14 2018, 10:18 PM

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.

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

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.

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.

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

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.

That would be my preference.

michaelh updated this revision to Diff 29600.Mar 15 2018, 2:35 PM
michaelh marked an inline comment as done.
  • Apply suggested changes
michaelh marked 4 inline comments as done.Mar 15 2018, 2:52 PM

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?

markg added a comment.Mar 15 2018, 4:24 PM

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)

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.

ngraham edited the summary of this revision. (Show Details)Mar 26 2018, 7:27 PM
ngraham accepted this revision as: ngraham.Apr 10 2018, 5:11 AM

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.

I'd like to see the Information Panel's configuration moved into Dolphin's Configure window.

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.

I'd like to see the Information Panel's configuration moved into Dolphin's Configure window.

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.

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?

markg accepted this revision.Apr 11 2018, 9:49 AM
This revision is now accepted and ready to land.Apr 11 2018, 9:49 AM
This revision was automatically updated to reflect the committed changes.
elvisangelaccio reopened this revision.Apr 15 2018, 9:06 PM
elvisangelaccio added a subscriber: elvisangelaccio.

This patch should have bumped the minimum required version of baloo-widgets.

This revision is now accepted and ready to land.Apr 15 2018, 9:06 PM
elvisangelaccio requested changes to this revision.Apr 15 2018, 9:06 PM
This revision now requires changes to proceed.Apr 15 2018, 9:06 PM
michaelh added a comment.EditedApr 16 2018, 2:32 PM

This patch should have bumped the minimum required version of baloo-widgets.

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.

This patch should have bumped the minimum required version of baloo-widgets.

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

michaelh updated this revision to Diff 32550.Apr 19 2018, 11:29 AM
  • Bump minimum required version of baloo-widgets
michaelh edited the summary of this revision. (Show Details)Apr 19 2018, 11:30 AM
elvisangelaccio commandeered this revision.May 12 2018, 6:54 PM
elvisangelaccio edited reviewers, added: michaelh; removed: elvisangelaccio.

Fixed with 2912894d4f13.

Restricted Application added a subscriber: kfm-devel. · View Herald TranscriptMay 12 2018, 6:54 PM
elvisangelaccio abandoned this revision.May 12 2018, 6:55 PM