Closed
Bug 1023735
Opened 10 years ago
Closed 10 years ago
[Settings] Support new time format settings
Categories
(Firefox OS Graveyard :: Gaia::Settings, defect)
Tracking
(feature-b2g:2.1)
People
(Reporter: arthurcc, Assigned: gasolin)
References
Details
(Whiteboard: [p=5] [ETA:8/15])
Attachments
(4 files, 1 obsolete file)
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.
Reporter | ||
Comment 1•10 years ago
|
||
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)
Comment 2•10 years ago
|
||
Hi Arthur - we don't have plans to refactor mozL10n.DateTimeFormat in the near future.
Flags: needinfo?(stas)
Reporter | ||
Updated•10 years ago
|
Blocks: settings-2.1
Reporter | ||
Comment 3•10 years ago
|
||
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
Assignee | ||
Comment 4•10 years ago
|
||
let's refactor date/time panel in bug 973441 first
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
I will let the developer to comment.
Flags: needinfo?(timdream) → needinfo?(gasolin)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [p=5]
Target Milestone: --- → 2.1 S2 (15aug)
Assignee | ||
Comment 9•10 years ago
|
||
UI spec from bug 903683
Assignee | ||
Comment 10•10 years ago
|
||
WIP, other fields are tuned to spec, need implement Time Format Fields
Updated•10 years ago
|
feature-b2g: --- → 2.1
Whiteboard: [p=5] → [p=5] [ETA:8/15]
Comment 12•10 years ago
|
||
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.
Assignee | ||
Comment 13•10 years ago
|
||
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
Assignee | ||
Comment 14•10 years ago
|
||
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)
Reporter | ||
Comment 15•10 years ago
|
||
The second approach makes more sense, please go for it, thanks!
Flags: needinfo?(arthur.chen)
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8469879 [details]
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)
Reporter | ||
Comment 17•10 years ago
|
||
Comment on attachment 8469879 [details]
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+
Assignee | ||
Comment 18•10 years ago
|
||
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)
Assignee | ||
Comment 19•10 years ago
|
||
Comment on attachment 8471291 [details]
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)
Reporter | ||
Comment 20•10 years ago
|
||
Comment on attachment 8471291 [details]
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)
Assignee | ||
Comment 21•10 years ago
|
||
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.
Assignee | ||
Comment 22•10 years ago
|
||
Comment on attachment 8471291 [details]
PR3
also fixed date/time picker disabled in no SIM card state, please kindly review it again. Thanks!
Attachment #8471291 -
Flags: review?(arthur.chen)
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8471291 [details]
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)
Reporter | ||
Comment 24•10 years ago
|
||
Comment on attachment 8471291 [details]
PR3
Please check my comments in github.
Attachment #8471291 -
Flags: review?(arthur.chen)
Assignee | ||
Comment 25•10 years ago
|
||
Comment on attachment 8471291 [details]
PR3
comment addressed, please kindly review it again.
Attachment #8471291 -
Flags: review?(arthur.chen)
Reporter | ||
Comment 26•10 years ago
|
||
Comment on attachment 8471291 [details]
PR3
r=me, thanks!
Attachment #8471291 -
Flags: review?(arthur.chen) → review+
Assignee | ||
Comment 27•10 years ago
|
||
merged to gaia master https://github.com/mozilla-b2g/gaia/commit/5e074831f9ddacdf6f622a6dffaecb626f740be8
thanks!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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-]
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-verify-] → [QAnalyst-Triage?][QAnalyst-verify-]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Comment 29•10 years ago
|
||
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)
Assignee | ||
Comment 30•10 years ago
|
||
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)
Reporter | ||
Comment 31•10 years ago
|
||
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)
Comment 32•10 years ago
|
||
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•10 years ago
|
||
(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)
Comment 34•10 years ago
|
||
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)
Comment 35•10 years ago
|
||
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)
Comment 36•10 years ago
|
||
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•10 years ago
|
||
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)
Comment 38•10 years ago
|
||
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•10 years ago
|
||
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.
Description
•