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

[Nathaniel Graham]

ngraham marked an inline comment as done.
ngraham added inline comments.

INLINE COMMENTS

> Zren wrote in configAppearance.qml:137
> 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

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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: cfeck, totte, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Chris Holland]

Zren added inline comments.

INLINE COMMENTS

> configAppearance.qml:137
> +icon.name: "preferences-desktop-locale"
> +onClicked: KCMShell.open("formats.desktop")
> +}

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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: cfeck, totte, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:74f0f2594655: [Digital Clock] Replace 12/24hr tri-state 
checkbox in config UI with combobox (authored by ngraham).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19230?vs=52847=52857

REVISION DETAIL
  https://phabricator.kde.org/D19230

AFFECTED FILES
  applets/digital-clock/package/contents/ui/configAppearance.qml

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: cfeck, totte, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  In D19230#422118 , @cfeck wrote:
  
  > 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.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: cfeck, totte, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Christoph Feck]

cfeck added a comment.


  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.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: cfeck, totte, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Noah Davis]

ndavis added a comment.


  In D19230#422088 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: totte, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: totte, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Noah Davis]

ndavis added a comment.


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

REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: totte, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Noah Davis]

ndavis accepted this revision.
ndavis added a comment.


  I think so

REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: totte, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  All good now?

REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: totte, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Nathaniel Graham]

ngraham updated this revision to Diff 52847.
ngraham added a comment.


  - Change string to "Use Region Defaults"
  - Add a button that opens the Formats KCM

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19230?vs=52329=52847

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

AFFECTED FILES
  applets/digital-clock/package/contents/ui/configAppearance.qml

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: totte, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Hans Tovetjärn]

totte added a comment.


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

REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: totte, ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Noah Davis]

ndavis added a comment.


  In D19230#421924 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Andres Betts]

abetts added a comment.


  In D19230#421336 , @ngraham wrote:
  
  > Sounds good. What should the string be? "Use region's default setting"?
  >
  > F6643429: Screenshot_20190227_114945.png 

  
  
  Yes, I am with @ndavis to use Regio

REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


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


REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Andres Betts]

abetts added a comment.


  In D19230#421229 , @ndavis wrote:
  
  > In D19230#421215 , @abetts wrote:
  >
  > > 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.F6643106: Screenshot_20190227_104128.png 

  
  
  That seems right to me

REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Noah Davis]

ndavis added a comment.


  In D19230#421215 , @abetts wrote:
  
  > 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.F6643106: Screenshot_20190227_104128.png 


REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Noah Davis]

ndavis accepted this revision.
ndavis added a comment.


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

REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson, ndavis
Cc: ndavis, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Andres Betts]

abetts added a comment.


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

REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg, davidedmundson
Cc: abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  Ping!

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham updated this revision to Diff 52329.
ngraham edited the test plan for this revision.
ngraham added a comment.


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

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19230?vs=52323=52329

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

AFFECTED FILES
  applets/digital-clock/package/contents/ui/configAppearance.qml

To: ngraham, #plasma, Zren, #vdg
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Chris Holland]

Zren added a comment.


  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.
  

  Force the clock to use 12/24 hour time, instead of following 
the user locale.
  1

  
  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 Qt Docs.
  
  - 
https://doc.qt.io/qt-5.11/qml-qtquick-controls2-combobox.html#currentIndex-prop
  
  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
}
  
  
  
  - https://doc.qt.io/qt-5.11/qml-qtquick-controls2-combobox.html#model-prop
  - https://doc.qt.io/qt-5.11/qml-qtquick-controls2-combobox.html#textRole-prop

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19230

To: ngraham, #plasma, Zren, #vdg
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham created this revision.
ngraham added reviewers: Plasma, Zren.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ngraham requested review of this revision.

REVISION 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.
  
  BUG: 402487
  FIXED-IN: 5.16.0

TEST PLAN
  New UI: F6630383: Combobox.png 
  
  Tested functionality with `en_US` locale. Check out the clock in the 
bottom-right corner of
  the following screenshots:
  
  Default state: use locale default: F6630384: Use locale default.png 

  
  Force 12 hour time: F6630386: Force 12 hour.png 

  
  Force 24-hour time F6630391: Force 24 hour.png 


REPOSITORY
  R120 Plasma Workspace

BRANCH
  replace-tristate-checkbox-with-combobox (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19230

AFFECTED FILES
  applets/digital-clock/package/contents/ui/configAppearance.qml

To: ngraham, #plasma, Zren
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart