1187539 - Add "default" option to 12/24 hour format

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

This is a follow up to bug 903683.

In bug 903683 we added an option to control 12/24 time format in Gaia.

The current UI allows the user to set one of the two options, but in fact, there's a third option - automatic (or: default).

With the current migration to Intl (bug 1170963) we can finally start using it.

The default option means that the system will follow the default setting for this value for a given locale. Intl has this defined for each locale, so we can just use that as a default.

The code is ready to adopt it, all we need is UX in Settings to be able to set/reset to the third option.

My minimal proposal is to have the selection list in Settings have three values:

Settings > Date & Time > Time Format
 - Automatic
 - 12-hour
 - 24-hour

which will translate to 'locale.hour12' setting to be:
 - undefined (for Automatic)
 - true (for 12-hour)
 - false (for 24-hour)

This in turn will be properly consumed by Intl API config options.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Harly, can you help with the UX decision in this bug or redirect to someone who can?

Would you be ok with adding third option to that selection list and making it a default one?

I believe that from the user experience standpoint that achieves two things:

1) It makes the UI represent reality better. Currently, until user manipulates with the selection list, the system will use the automatic option, but it's not visible in the UI.

2) Once the user manually select 12/24 format, there's no way for the user to switch back to the automatic selection. So he cannot undo and get back to the default model in which Gaia chooses the 12/24 format based on the default for the selected language.

Comment 2

[Harly Hsu[:harly]]

Hi Zibi,
I am the UX owner for Settings, so probably I should be the one to answer this bug.

Basically I agree with your point that we don't provide a way for user to switch back to automatic will be a pain for some users. However, by providing three options to the users probably won't be that clear as user will not be able to know what the current time format is because it will just display "Default". So I am thinking that we do this the same as "Set automatically" in Date & Time, and to have a toggle switch for time format as well. If users want to set format manually, they will need to disable the toggle, and then tap the value selector to change 12/24 format. That's my thoughts on this, thoughts?

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

yeah, sure! The only thing, I don't see "Set automatically" toggle in Settings > Date & Time on master.
Did I miss something?

Comment 4

[Harly Hsu[:harly]]

You will need to have a valid sim card on your phone to be able to "Set automatically".

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

That sounds good! I'll ping you when I have some code and screenshots.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] zbraniecki:1187539-default-hour12 > mozilla-b2g:master

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8652653 [details] [review]
[gaia] zbraniecki:1187539-default-hour12 > mozilla-b2g:master

Stas, Evelyn, can you take a look at this patch and let me know how it looks?

The basic change is that it turn the "undefined" state of the locale.hour12 perf into a meaningful one - "automatic time format".

The nice side effect is that we're removing the scenario where an app wants to reformat time on timeformatchange and languageschange, so it sets two observers but because languageschange triggers timeformatchange it gets fired twice.

With this patch, languageschange does not trigger timeformatchange because hour12 is either manually set or remains "undefined".

The trick is that because mozSettings does not allow for removal of a pref, I have to mock the "null" value as undefined.
Once we move mozHour12 to the platform (bug 1172732), it will just always return true/false/undefined.

The reason I stick to undefined instead of "null" is because this allows me to just do:

toLocaleString(navigator.languages, {
  hour12: navigator.mozHour12,
  hour: 'numeric'
});

and if mozHour12 is undefined, it will pick the default for the given language.

I know I need to write tests, but I think the patch works well enough to get your feedback on it before I start working on tests.

Comment 8

[Evelyn Hung]

Hi zibi, I'm a bit busy on other higher priority stuff, would need a few days to response here. Sorry about that.

Comment 9

[Staś Małolepszy :stas]

Comment on attachment 8652653 [details] [review]
[gaia] zbraniecki:1187539-default-hour12 > mozilla-b2g:master

I left a few comments in the PR, but overall I like the change and how `undefined` has a meaning that's true to its name.  Nice work!

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot in en-US with automatic on

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot in en-US with automatic off and 24h on

Comment 12

[Harly Hsu[:harly]]

I don't know about having 2 "Set Automatically", probably will be a bit confusing for the user. NI Matej to see if there is a better phrase for this.

Comment 13

[Matej Novak [:matej]]

In this case I'm not clear on the benefit of the user being able to set the time format automatically. It could be something like "Use default," but why wouldn't we just let them pick it themselves?

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Matej Novak [:matej] from comment #13)
> In this case I'm not clear on the benefit of the user being able to set the
> time format automatically. It could be something like "Use default," but why
> wouldn't we just let them pick it themselves?

The logic behind it is that we do have three states of this action and current UI breaks the two paradigms:

1) Ability to revert accidental action
2) Allowing the user to reach any of the three states

With the current UI the user starts interacting with the UI when the locale.hour12 is undefined, and after interacting with the toggle he can never get back to that state.

My understanding is that users may want to try the feature to see what it does, and *most* users who will touch that toggle will want to get back to automatic setting that follows the default for their language.

The problem here is that they cannot - the only way to achieve it is to reset the phone.

Originally I was thinking about three state selection for this UI "Automatic|24 hour|12 hour", but Harly suggested trying the "Set automatic" checkbox first.

Does my logic make sense here?

Comment 15

[Matej Novak [:matej]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #14)
> 
> Does my logic make sense here?

Yes, it does. Thanks so much for the extra explanation.

Based on that, I think "Use default" would work.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot in en-US with automatic on

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot in en-US with automatic off

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot in en-US with automatic off and 24h on

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

Evelyn, when you may have time to give me feedback on that?
I'd like to verify if I'll be able to get it in 2.5 (so that I can design IntlHelper according to the new timeformatchange/languagechange split - bug 1191011)

Comment 20

[Harly Hsu[:harly]]

Attachment: 2015-09-14-10-05-08.png

Hi Zibi,
After testing the patch you've provided, everything seems good except 2 minor things:
1. The Time Format when disabled should not have a on press state (see image)
2. Is "Use Default" enabled or disabled by default?

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

1) Sure, I was planning to fix it when I'm working on the patch after the round of feedback
2) In my thinking it should be enabled by default (so locale.hour12 is undefined until user changes it)

Comment 22

[Evelyn Hung]

Comment on attachment 8652653 [details] [review]
[gaia] zbraniecki:1187539-default-hour12 > mozilla-b2g:master

Sorry for my very late feedback! Most of my concerns had been raised by Harly and stas, so looks good to me so far. Thanks. :)

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8652653 [details] [review]
[gaia] zbraniecki:1187539-default-hour12 > mozilla-b2g:master

Excellent, thank you!

I updated the patch and added panel tests for the new UI pieces. I also played with it extensively and it seems to work really well.

One thing to notice is that I am changing shared date_time_helper code which is used for timeformat in other apps, so if you only reinstall Settings app, you may see inconsistency with System clock. (like, you'll change to "use default" and it should be hour12, but system clock will show 24h) - this goes away when you reinstall System app as well with this patch.

Comment 24

[Harly Hsu[:harly]]

Comment on attachment 8660394 [details]
Screenshot in en-US with automatic off and 24h on

LGTM, thanks:)

Comment 25

[Evelyn Hung]

Comment on attachment 8652653 [details] [review]
[gaia] zbraniecki:1187539-default-hour12 > mozilla-b2g:master

Everything is good. Please also add an initial value in common-settings.json for locale.hour12, so the default is really 'undefined'.

Comment 26

[Zibi Braniecki [:zbraniecki][:gandalf]]

Commit: https://github.com/mozilla-b2g/gaia/commit/636b9296bc8b5e14bc3184b26616a2d21f8d3e00

Thanks Evelyn, Stas!

Comment 27

[Zibi Braniecki [:zbraniecki][:gandalf]]

I had to land a small follow-up for a calendar test that assume default mozHour12 - https://github.com/mozilla-b2g/gaia/commit/42b564dac8c7b2fea6a02c55005e7547b0101c7c