[Digital Clock] Replace 12/24hr tri-state checkbox in config UI with combobox
ClosedPublic

Authored by ngraham on Feb 22 2019, 5:43 PM.

Details

Summary

The use of a tri-state checkbox for the three possible states (12hr, 24hr, use locale
default) is not ideal because it violates the convention regarding what tri-state
checkboxes are used for: nested lists where some sub-items can be unselected.

This patch replaces it with a combobox that clearly indicates all three states.

While we're at it, we add a button to open the Formats KCM in case people want to change their region.

BUG: 402487
FIXED-IN: 5.16.0

Test Plan

Tested functionality with en_US locale. Check out the clock in the bottom-right corner of
the following screenshots:

Default state: "Use Region Defaults":

Force 12 hour time:

Force 24-hour time:

Diff Detail

Repository
R120 Plasma Workspace
Branch
replace-tristate-checkbox-with-combobox (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8705
Build 8723: arc lint + arc unit
ngraham created this revision.Feb 22 2019, 5:43 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 22 2019, 5:43 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.Feb 22 2019, 5:43 PM
ngraham edited the test plan for this revision. (Show Details)Feb 22 2019, 5:46 PM
ngraham added a reviewer: VDG.
Zren added a comment.EditedFeb 22 2019, 6:08 PM

https://doc.qt.io/qt-5/qt.html#CheckState-enum

i18n("Use locale default") should be in the middle (currentIndex=1). 12h should be currentIndex=0, and 24h should be currentIndex=2

https://github.com/KDE/plasma-workspace/blob/master/applets/digital-clock/package/contents/config/main.xml#L64

People upgrading from 5.15 will have either "force 12h" (0) or "force 24h" (2) serialized to file. KConfig doesn't serialize the default values to file (1).

Make sure to test changing contents/config/main.xml to simulate "loading" a different config value from file.

<entry name="use24hFormat" type="UInt">
      <label>Force the clock to use 12/24 hour time, instead of following the user locale.</label>
      <default>1</default>
</entry>

with 0 and 2 to make sure it selects 12h/24h when the config opens.

Right now the config should be selecting 12h (currentIndex=1), however currentIndex is the default currentIndex=0 as mentioned in the Qml Docs. Edit: I tested the patch in plasmoidviewer, and it selected "12h".


If you really want the "locale default" at the top, you'll need to use the following pattern for the model

textRole: "label"
model: [
  { label: i18n("Use locale default"), value: 1 },
  { label: i18n("12-hour"), value: 0 },
  { label: i18n("24-hour"), value: 2 },
]
Component.onCompleted: {
  // select index with value == cfg_use24hFormat
  // watch cfg_use24hFormat for changes
}
ngraham updated this revision to Diff 52329.Feb 22 2019, 6:25 PM
ngraham edited the test plan for this revision. (Show Details)

Put "Use locale default" in the middle so everything works properly (it looks kinda cool there too)

Thanks @Zren. Moving "Use locale default" to the middle position makes everything work perfectly. I tested the upgrade use cases and the combobox is always pre-populated with the correct value.

davidedmundson accepted this revision.Feb 27 2019, 3:28 PM
This revision is now accepted and ready to land.Feb 27 2019, 3:28 PM
abetts added a subscriber: abetts.Feb 27 2019, 3:29 PM

Do you think that "locale" is a very specific term? Could it be different?

ndavis accepted this revision.Feb 27 2019, 3:38 PM
ndavis added a subscriber: ndavis.

+1

This is a much more usable design and more in line with our HIG.

Do you think that "locale" is a very specific term? Could it be different?

The alternative would be "region", which is the term we use in the related settings KCM.

Do you think that "locale" is a very specific term? Could it be different?

The alternative would be "region", which is the term we use in the related settings KCM.

That seems right to me

Sounds good. What should the string be? "Use region's default setting"?

Sounds good. What should the string be? "Use region's default setting"?

Yes, I am with @ndavis to use Regio

I mean, what do you think about the specific string "Use region's default setting". Too wordy? Just right?

I mean, what do you think about the specific string "Use region's default setting". Too wordy? Just right?

You could use use "Default" and maybe have a button to open the Formats KCM nearby.

totte added a subscriber: totte.Feb 28 2019, 2:58 PM

What about "Use region default", if it is to replace "Use locale default"?

ngraham updated this revision to Diff 52847.Feb 28 2019, 5:51 PM
  • Change string to "Use Region Defaults"
  • Add a button that opens the Formats KCM
ngraham edited the test plan for this revision. (Show Details)Feb 28 2019, 5:53 PM

All good now?

ngraham edited the summary of this revision. (Show Details)Feb 28 2019, 5:53 PM
ndavis accepted this revision.Feb 28 2019, 6:20 PM

I think so

Can you hide the button to open the Formats KCM when "Use Region Defaults" is not selected?

We could, but is that really necessary? In general we try to avoid having UI elements dynamically show and hide themselves outside of the narrowly-defined exception of when a UI element is inapplicable to the currently active hardware. That condition doesn't apply here, so what we would do instead is enable and disable the button, which I think would be a bit weird.

We could, but is that really necessary? In general we try to avoid having UI elements dynamically show and hide themselves outside of the narrowly-defined exception of when a UI element is inapplicable to the currently active hardware. That condition doesn't apply here, so what we would do instead is enable and disable the button, which I think would be a bit weird.

Fair point. I just think it's not necessary to show the button if you're not using the setting that uses the settings behind the button.

cfeck added a subscriber: cfeck.Feb 28 2019, 8:39 PM

Disabling is preferred to hiding. If there is a widget, but not enabled, you are eager to learn how to enable it. If you hide it, you won't even know it exists.

Disabling is preferred to hiding. If there is a widget, but not enabled, you are eager to learn how to enable it. If you hide it, you won't even know it exists.

Right. My point is that it doesn't really make sense to disable the button when the combobox is something other than "Use Region Defaults". That would just be odd and confusing.

This revision was automatically updated to reflect the committed changes.
Zren added inline comments.Feb 28 2019, 10:02 PM
applets/digital-clock/package/contents/ui/configAppearance.qml
136

Was afk today sorry.

Do we need visible: KCMShell.authorize("formats.desktop").length > 0 here? It hides the contextmenu item in main.qml if the user doesn't have permission.

https://github.com/KDE/plasma-workspace/blob/master/applets/digital-clock/package/contents/ui/main.qml#L109

ngraham marked an inline comment as done.Mar 1 2019, 3:49 PM
ngraham added inline comments.
applets/digital-clock/package/contents/ui/configAppearance.qml
136

Good Idea, I'll push that in a few minutes.