1023735 - [Settings] Support new time format settings

Status: RESOLVED FIXED

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

Description

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: Date & Time 01.pdf

Users should be able to set date/time format separately regardless the language setting.

The attachment is the draft proposal from the UX team and we can move forward based on it.

Comment 1

[Arthur Chen [:arthurcc][inactive after 6/30]]

Hi Stas, as I know the l10n team is working on refactoring the l10n library. May I know is there any plan regarding the date/time format?

Comment 2

[Staś Małolepszy :stas]

Hi Arthur - we don't have plans to refactor mozL10n.DateTimeFormat in the near future.

Comment 3

[Arthur Chen [:arthurcc][inactive after 6/30]]

Only handles time format in 2.1. Once bug 846200 lands, we can provide a permission of access to the settings field of time format to gaia apps.

Comment 4

[[e-mail]]

let's refactor date/time panel in bug 973441 first

Comment 5

[(no longer active)]

What are we planning to do for the default value of this field?  While in the UX spec it is shown as a binary setting, I think it should be a ternary under the hood, with "default", "12 hour" and "24 hour".  "default" should be used if the user hasn't touched the setting explicitly, and when you change your language, we need to update this field in the UI if its underlying value is "default" to make sure that we keep respecting the locale's default time format if the user hasn't chosen to override it.

Tim, what do you think?

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I will let the developer to comment.

Comment 7

[[e-mail]]

per offline discussion with arthur, though the UI looks like binary setting, we'd plan to save the value as string, so it will be future-proved for more flexible timer format.

Like TimeZone settings, we have a `userSelectTimeZone` as inner settings. I'd expect there's something similar to that for `userSelectDateFormat`, `userSelectTimeFormat` as well.

Comment 8

[(no longer active)]

Thanks, Fred!  I am not familiar enough with the details of your implementation, but it sounds like you have already thought about this, so I trust your judgement here.

Comment 9

[[e-mail]]

Attachment: ui_spec.png

UI spec from bug 903683

Comment 10

[[e-mail]]

Attachment: pull request redirect to github

WIP, other fields are tuned to spec, need implement Time Format Fields

Comment 12

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Thanks Howie and Fred for setting the ETA. 
There are various tasks depending on this, so please do secure it in sprint2. 
Sprint3 is the last development cycle for people to finish those remaining feature works.

Comment 13

[[e-mail]]

Attachment: PR2

WIP, Time Format Fields with correct options

will add following fields dependency rules

1. change timeZone -> redraw time field with user pref
2. change timeFormat -> update time field with user pref

3. change date field -> redraw dateFormat with user pref
4. change dateFormat -> update date field with user pref

we don't need following rule because we only care hour12 instead of real time:

x change Time -> redraw TimeFormat

Comment 14

[[e-mail]]

I've thinking 2 approaches to address comment 5 `user selection` issue.

1st is use `hour12` with [null | true | false] state (like we did in CMAS toggle patch). If user pick an option, we set hour12 to true/false;
If language changed,  we set hour12 to null, so we can distinguish the difference.


2nd. is use `hour12` with only [true | false] state.
If language changed (in FTU or settings), set date-format and hour12 based on language locale properties(dateTimeFormat_%x, shortTimeFormat).
For time formating, keep the origin `shortTimeFormat` and add `shortTimeFormat12`/`shortTimeFormat24` for explicit hour12/24 string. So we could have hour12 boolean by comparing `shortTimeFormat` and newly `shortTimeFormat12`.

I'd tend to the 2nd approach, which is simpler and need less runtime judgement logic.

With this approach, the extra work for vender that not use en-US as default language is needed: they should also set date-format and hour12 to correspondent value in their customized build.

@arthur does this sound good to you?

Comment 15

[Arthur Chen [:arthurcc][inactive after 6/30]]

The second approach makes more sense, please go for it, thanks!

Comment 16

[[e-mail]]

Comment on attachment 8469879 [details] [review]
PR2

The patch has implement all panel interactions and test fine on device.
Still need add navigator.hour12 shim and more test cases.

So set feedback? to make sure things in the right direction.

Comment 17

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8469879 [details] [review]
PR2

Looks good! Note that we only ship time format settings in this release. As the date format API is totally not discussed and all gaia app do not plan to reflect the date format change, I would recommend only implementing the time format part.

Comment 18

[[e-mail]]

Attachment: PR3

Removed dateFormat and add navigator.mozHour12 shim as well.

The related system patch that use this shim is in https://github.com/mozilla-b2g/gaia/pull/22757 (WIP)

Comment 19

[[e-mail]]

Comment on attachment 8471291 [details] [review]
PR3

@francisco please help review the FTU part. the related commit is at https://github.com/gasolin/gaia/commit/ae490a0b7817f20431b31aaf159f4ec0cac1f751

as Comment 14, we need set `hour12` value based on locale when language changed in ftu and settings.

Comment 20

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8471291 [details] [review]
PR3

Thanks for the patch. Please check my comments in github. One of the issues I found is that when there is no sim card, the selectors for adjusting date and time are disabled.

BTW, for the shim, please ensure it works in apps before merging.

Comment 21

[[e-mail]]

Thank Arthur for review. comments are addressed. Will fix the selectors disabled in no SIM status issue tomorrow.


To test shim, run

```
curl https://github.com/mozilla-b2g/gaia/pull/22757 | git apply
```

and reset-gaia. change the timeFormat and the status bar clock will be changed.

Comment 22

[[e-mail]]

Comment on attachment 8471291 [details] [review]
PR3

also fixed date/time picker disabled in no SIM card state, please kindly review it again. Thanks!

Comment 23

[[e-mail]]

Comment on attachment 8471291 [details] [review]
PR3

Since this settings and API shim part is a blocker for following bugs.

I'd move ftu related part to another patch https://github.com/mozilla-b2g/gaia/pull/22813 and fired bug 1053024 for it.

Comment 24

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8471291 [details] [review]
PR3

Please check my comments in github.

Comment 25

[[e-mail]]

Comment on attachment 8471291 [details] [review]
PR3

comment addressed, please kindly review it again.

Comment 26

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8471291 [details] [review]
PR3

r=me, thanks!

Comment 27

[[e-mail]]

merged to gaia master https://github.com/mozilla-b2g/gaia/commit/5e074831f9ddacdf6f622a6dffaecb626f740be8

thanks!

Comment 28

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Unable to verify this bug. There are multiple back end issues I cannot check within the depends on bugs of this bug. I will not be able to verify this bug until all of the depends on (bug 968686, bug 846200, and bug 973441 ) are closed out and verified.

Comment 29

[Julien Wajsberg [:julienw]]

Arthur, Fred, do you know of any bug that would implement this in Gecko? If yes, do you know how it moves forward? If no, can we file one?

The shim has performance issues because of the use of mozSettings :/

see bug 1118214.

Comment 30

[[e-mail]]

I think there's no further plan yet for implement this in Gecko.

ehsan, is there any progress from gecko part?

Comment 31

[Arthur Chen [:arthurcc][inactive after 6/30]]

As far as I know there is no bug tracking this. As for the performance issue, wondering is date_time_helper the only part that makes queries to the settings db in the starting path of the messaging app?

Comment 32

[Julien Wajsberg [:julienw]]

I specifically measured this JS file using console.time() at the start and console.timeEnd() at the end, and there is a penalty just by inserting/executing the file.

Comment 33

[(no longer active)]

(In reply to Fred Lin [:gasolin] from comment #30)
> I think there's no further plan yet for implement this in Gecko.
> 
> ehsan, is there any progress from gecko part?

There is no work ongoing for this on the Gecko side that I'm aware of.

Comment 34

[Julien Wajsberg [:julienw]]

But, do we want it, or do we want to wait for the JS i18n API? What's the plan?

Sorry to be dumb, but I'd really want to eventually get rid of the mozSettings hack here.

Comment 35

[(no longer active)]

My impression was that this was on the list of Tako features that was deprioritized when the Tako project was canceled.  If that changed, nobody told me.  :/  At any rate, I won't have any time to work on this during Q1.  Andrew, what's the priority of this?

Comment 36

[Julien Wajsberg [:julienw]]

Note that my only concern is about performance; if we decide to disable the feature in sake of performance, I'd be happy too.

Another possibility is to make mozSettings faster ;)

Comment 37

[Andrew Overholt [:overholt]]

I'll talk to Martin to ensure this is on our radar.  Unless someone says it's urgent, it'll have to be Q2, though.

Comment 38

[(no longer active)]

Why can't we make mozSettings faster?  I have an extremely hard time believing this is the only thing affected by the perf issue.  ;-)

Comment 39

[Julien Wajsberg [:julienw]]

Yeah, Alexandre Lissy filed bug 1122570 and I filed bug 1122649 about the specific issue we're facing here.