Closed Bug 1023735 Opened 6 years ago Closed 5 years ago

[Settings] Support new time format settings

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.1)

RESOLVED FIXED
2.1 S2 (15aug)
feature-b2g 2.1

People

(Reporter: arthurcc, Assigned: gasolin)

References

Details

(Whiteboard: [p=5] [ETA:8/15])

Attachments

(4 files, 1 obsolete file)

1.04 MB, application/pdf
Details
125.51 KB, image/png
Details
PR2
46 bytes, text/x-github-pull-request
arthurcc
: feedback+
Details | Review
PR3
46 bytes, text/x-github-pull-request
arthurcc
: review+
Details | Review
Attached file 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.
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?
Flags: needinfo?(stas)
Hi Arthur - we don't have plans to refactor mozL10n.DateTimeFormat in the near future.
Flags: needinfo?(stas)
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.
Depends on: 846200
Summary: [Settings] Support new date/time format settings → [Settings] Support new time format settings
Blocks: 903683
let's refactor date/time panel in bug 973441 first
Assignee: nobody → gasolin
Depends on: 968686, 973441
Blocks: 1035754
Blocks: 1035757
Blocks: 1035759
Blocks: 1035760
Blocks: 1035761
Blocks: 1035762
Blocks: 1035763
Blocks: 1035769
Blocks: 1035775
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?
Flags: needinfo?(timdream)
I will let the developer to comment.
Flags: needinfo?(timdream) → needinfo?(gasolin)
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.
Flags: needinfo?(gasolin)
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.
Status: NEW → ASSIGNED
Whiteboard: [p=5]
Target Milestone: --- → 2.1 S2 (15aug)
Attached image ui_spec.png
UI spec from bug 903683
Attached file pull request redirect to github (obsolete) —
WIP, other fields are tuned to spec, need implement Time Format Fields
Duplicate of this bug: 808842
feature-b2g: --- → 2.1
Whiteboard: [p=5] → [p=5] [ETA:8/15]
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.
Attached file 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
Attachment #8464625 - Attachment is obsolete: true
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?
Flags: needinfo?(arthur.chen)
The second approach makes more sense, please go for it, thanks!
Flags: needinfo?(arthur.chen)
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.
Attachment #8469879 - Flags: feedback?(arthur.chen)
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.
Attachment #8469879 - Flags: feedback?(arthur.chen) → feedback+
Attached file 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)
Attachment #8471291 - Flags: review?(arthur.chen)
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.
Attachment #8471291 - Flags: review?(francisco)
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.
Attachment #8471291 - Flags: review?(arthur.chen)
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 on attachment 8471291 [details] [review]
PR3

also fixed date/time picker disabled in no SIM card state, please kindly review it again. Thanks!
Attachment #8471291 - Flags: review?(arthur.chen)
Blocks: 1053024
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.
Attachment #8471291 - Flags: review?(francisco)
Comment on attachment 8471291 [details] [review]
PR3

Please check my comments in github.
Attachment #8471291 - Flags: review?(arthur.chen)
Comment on attachment 8471291 [details] [review]
PR3

comment addressed, please kindly review it again.
Attachment #8471291 - Flags: review?(arthur.chen)
Comment on attachment 8471291 [details] [review]
PR3

r=me, thanks!
Attachment #8471291 - Flags: review?(arthur.chen) → review+
merged to gaia master https://github.com/mozilla-b2g/gaia/commit/5e074831f9ddacdf6f622a6dffaecb626f740be8

thanks!
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1053669
Blocks: 1054132
Blocks: 1054135
Blocks: 1059162
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.
QA Whiteboard: [QAnalyst-verify-]
QA Whiteboard: [QAnalyst-verify-] → [QAnalyst-Triage?][QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
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.
Flags: needinfo?(gasolin)
Flags: needinfo?(arthur.chen)
I think there's no further plan yet for implement this in Gecko.

ehsan, is there any progress from gecko part?
Flags: needinfo?(gasolin) → needinfo?(ehsan)
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?
Flags: needinfo?(arthur.chen)
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.
(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.
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
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?
Flags: needinfo?(ehsan) → needinfo?(overholt)
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 ;)
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.
Flags: needinfo?(overholt)
Why can't we make mozSettings faster?  I have an extremely hard time believing this is the only thing affected by the perf issue.  ;-)
Yeah, Alexandre Lissy filed bug 1122570 and I filed bug 1122649 about the specific issue we're facing here.
You need to log in before you can comment on or make changes to this bug.