Closed
Bug 1396211
Opened 7 years ago
Closed 7 years ago
Clean up code for dates in aboutTelemetry.js
Categories
(Toolkit :: Telemetry, enhancement)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: flod, Assigned: flod)
Details
Attachments
(2 files)
Right now datetime in pings is displayed as "mm/dd/yyyy hh:mm:ss", with some hard coded logic and concatenations in aboutTelemetry.js http://searchfox.org/mozilla-central/rev/2aa0806c598ec433e431728f5ddd3a6847c1a417/toolkit/content/aboutTelemetry.js#169-210 There's also a lot of unused code around. shortDateString is completely unused since you're using date.toLocaleDateString() http://searchfox.org/mozilla-central/rev/2aa0806c598ec433e431728f5ddd3a6847c1a417/toolkit/content/aboutTelemetry.js#463
Comment 1•7 years ago
|
||
For user-facing datetime formatting, please, use mozIntl.createDateTimeFormat and use dateStyle and timeStyle. That API can optionally use OS regional preferences the user set.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
Comment on attachment 8903889 [details] Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code Zibi, does this make any sense in terms of JS and Intl? I don't want to change the format they're using ("date time, pingname"), and couldn't find a better way to do it. Using toLocaleString would get me "date, time, ping".
Attachment #8903889 -
Flags: feedback?(gandalf)
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8903889 [details] Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code https://reviewboard.mozilla.org/r/175668/#review180740 ::: toolkit/content/aboutTelemetry.js:429 (Diff revision 1) > + let timeFormat = { > + hour: '2-digit', > + minute: '2-digit', > + second: '2-digit', > + hour12: false > + }; Why do you force `hour12: false` here? That's not intl if hardcode something that should depend on locale :) ::: toolkit/content/aboutTelemetry.js:434 (Diff revision 1) > + }; > for (let p of this._archivedPings) { > pingTypes.add(p.type); > - let date = new Date(p.timestampCreated); > - let datetext = date.toLocaleDateString() + " " + shortTimeString(date); > + let pingDate = new Date(p.timestampCreated); > + let datetext = (new Intl.DateTimeFormat([],dateFormat).format(pingDate)) + > + " " + (new Intl.DateTimeFormat([],timeFormat).format(pingDate)); So, Intl.DateTimeFormat is the standardized part of JavaScript that allows us to generate internationalized date/time. So your use of that API is technically correct. Now, at Mozilla, we have an extended version of that API that is chrome-only, and allows us to do more. It's called mozIntl.DateTimeFormat. Whenever you work within Firefox chrome, I recommend using mozIntl.DateTimeFormat over Intl.DateTimeFormat. The mozIntl version optionally looks up OS regional preferences following user adjustments. Since Windows and Mac only allow the user to adjust their date/time formatting for styles short/medium/long/full - we have a new options `dateStyle` and `timeStyle`. If you use them, we'll look up into regional prefs. There's a convenient accessor to mozIntl - `Services.intl`. So, I'd suggest: ``` const dateText = Services.intl.createDateTimeFormat(undefined, {dateStyle: 'short'}).format(pingdate); const timeText = Services.intl.createDateTimeFormat(undefined, {timeStyle: 'long'}).format(pingdate); let text = `${dateText} ${timeText}, ${p.type}`. ``` You can toy with different values for dateStyle/timeStyle by opening browser console and calling the `Services.intl.createDateTimeFormat(undefined, {...}).format(Date.now())` Now, the other problem with your approach is that you hardcode the space between date and time making it forcefully left-to-right. Also, not every language uses space as a separator between date and time. So, you can imporve that partially by: ``` const dateText = Services.intl.createDateTimeFormat(undefined, {dateStyle: 'short', timeStyle: 'long'}).format(pingdate); let text = `${dateText}, ${p.type}`. ``` Now, mozIntl will look up date/time separator for the given locale and use it. In result your date and time text is now properly internationalized. But you still have the hardcoded LTR part about `, ${p.type}`. I'm not sure what to do about it. My instinct would be to try to have two semantic elements - one for date, one for type, but it doesn't seem like it'll work for `<option>` so maybe it's ok to leave it for now? ::: toolkit/content/aboutTelemetry.js:444 (Diff revision 1) > option.appendChild(content); > option.setAttribute("value", p.id); > option.dataset.type = p.type; > option.dataset.date = datetext; > > - if (date.toDateString() == todayString) { > + if (pingDate.toDateString() == today.toDateString()) { nit: forcing two dates to produce a string representation is very inefficient. On top of that, you're possibly doing this twice here. It would be better to get a representation of the date stripped of time: ``` const dateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0); const today = new Date(); today.setHours(0, 0, 0, 0); const yesterday = new Date(today); yesterday.setDate(today.getDate() - 1); if (dateWithoutTime === today) { ... } else if (dateWithoutTime === yesterday) { } ```
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8903889 [details] Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code https://reviewboard.mozilla.org/r/175668/#review180754 Thanks for taking the time Zibi, that's a lot of useful information :) I have the feeling I'm out of my depth here, but let's give it a shot.
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8903889 [details] Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code https://reviewboard.mozilla.org/r/175668/#review180740 > Why do you force `hour12: false` here? That's not intl if hardcode something that should depend on locale :) Because I'm trying to stay as close as possible to the current behavior, in case I'm not sure it would be my call to change the current time format. For example we could drop the hardcoded space and use: Services.intl.createDateTimeFormat(undefined, {dateStyle: 'short', timeStyle: 'short'}).format(Date.now()) But that would result in "9/3/17, 9:08 AM, PINGNAME" for en-US (double comma). We need to show seconds, which would turn into a "10:40:00 AM" for en-US, with the header showing something as long as "10/10/2017 10:00:00 AM", and that requires to reduce the font size even further (15px, maybe 14px). > nit: forcing two dates to produce a string representation is very inefficient. > > On top of that, you're possibly doing this twice here. > > It would be better to get a representation of the date stripped of time: > ``` > const dateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0); > const today = new Date(); > today.setHours(0, 0, 0, 0); > const yesterday = new Date(today); > yesterday.setDate(today.getDate() - 1); > > if (dateWithoutTime === today) { > ... > } else if (dateWithoutTime === yesterday) { > } > ``` As the rest of the code, I tried to move away as little as possible from the original (e.g. toLocaleString), but I think it makes sense to do it, and clean up things. Thanks.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Comment on attachment 8903889 [details] Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code Resetting flag. I kept the 24 hours format for now, in case it's up to a Telemetry peer to decide if the format could change to something else. I also find JS behavior with dates extremely confusing (the code fragment you suggested wouldn't work). const d = new Date(); date.setHours(0, 0, 0, 0); print(d); /* Sun Sep 03 2017 00:00:00 GMT+0200 (CEST) */ but const d = new Date().setHours(0, 0, 0, 0); print(d); /* 1504389600000 */
Attachment #8903889 -
Flags: feedback?(gandalf)
Comment 10•7 years ago
|
||
> the code fragment you suggested wouldn't work can you please be more specific? > but That's an artifact of how the API work. There's new Date API for JS in works - https://github.com/tc39/proposal-temporal - hope it'll be more consistent.
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10) > > the code fragment you suggested wouldn't work > > can you please be more specific? ``` const dateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0); const today = new Date(); today.setHours(0, 0, 0, 0); const yesterday = new Date(today); yesterday.setDate(today.getDate() - 1); if (dateWithoutTime === today) { ... } else if (dateWithoutTime === yesterday) { } ``` dateWithoutTime is a timestamp (for the artifact above), today and yesterday are not dateWithoutTime != today but dateWithoutTime === today.getTime()
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8903889 [details] Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code https://reviewboard.mozilla.org/r/175668/#review180762 ::: toolkit/content/aboutTelemetry.js:439 (Diff revision 3) > + }; > + > for (let p of this._archivedPings) { > pingTypes.add(p.type); > - let date = new Date(p.timestampCreated); > - let datetext = date.toLocaleDateString() + " " + shortTimeString(date); > + const pingDate = new Date(p.timestampCreated); > + const pingDateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0); I don't think you need this variable, nor `timeValue` or `dateValue`. You can compare two date objects. ::: toolkit/content/aboutTelemetry.js:441 (Diff revision 3) > for (let p of this._archivedPings) { > pingTypes.add(p.type); > - let date = new Date(p.timestampCreated); > - let datetext = date.toLocaleDateString() + " " + shortTimeString(date); > - let text = datetext + ", " + p.type; > + const pingDate = new Date(p.timestampCreated); > + const pingDateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0); > + const dateText = Services.intl.createDateTimeFormat(undefined, dateFormat).format(pingDate); > + const timeText = Services.intl.createDateTimeFormat(undefined, timeFormat).format(pingDate); You seem to decide to stick to manual list of tokens instead of `dateStyle` and `timeStyle` which I suggested in order to conform to the Firefox UI Intl practice of respecting user's Regional Preferneces in their OS. What's the reason for that? ::: toolkit/content/aboutTelemetry.js:442 (Diff revision 3) > pingTypes.add(p.type); > - let date = new Date(p.timestampCreated); > - let datetext = date.toLocaleDateString() + " " + shortTimeString(date); > - let text = datetext + ", " + p.type; > + const pingDate = new Date(p.timestampCreated); > + const pingDateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0); > + const dateText = Services.intl.createDateTimeFormat(undefined, dateFormat).format(pingDate); > + const timeText = Services.intl.createDateTimeFormat(undefined, timeFormat).format(pingDate); > + let datetimeText = `${dateText} ${timeText}`; So, you're still hardcoding LTR and space separator. Any reason for that? I provided you a snippet that internationalized date and time together.
Comment 13•7 years ago
|
||
> dateWithoutTime is a timestamp (for the artifact above), today and yesterday are not
Oh, then I'd recommend:
```
const dateWithoutTime = new Date(pingDate);
dateWithoutTime.setHours(0, 0, 0, 0);
```
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8903889 [details] Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code https://reviewboard.mozilla.org/r/175668/#review180756 ::: toolkit/content/aboutTelemetry.js:441 (Diff revision 3) > for (let p of this._archivedPings) { > pingTypes.add(p.type); > - let date = new Date(p.timestampCreated); > - let datetext = date.toLocaleDateString() + " " + shortTimeString(date); > - let text = datetext + ", " + p.type; > + const pingDate = new Date(p.timestampCreated); > + const pingDateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0); > + const dateText = Services.intl.createDateTimeFormat(undefined, dateFormat).format(pingDate); > + const timeText = Services.intl.createDateTimeFormat(undefined, timeFormat).format(pingDate); As explained, I don't think that's my call to change the date and time format for now, it should be up to a Telemetry peer. dateStyle: short has 2 numbers for the year, dateTime: long has AM/PM. At this point, it's probably clearer to NI and ask instead of continuining the back and forth between me and you ;-) ::: toolkit/content/aboutTelemetry.js:442 (Diff revision 3) > pingTypes.add(p.type); > - let date = new Date(p.timestampCreated); > - let datetext = date.toLocaleDateString() + " " + shortTimeString(date); > - let text = datetext + ", " + p.type; > + const pingDate = new Date(p.timestampCreated); > + const pingDateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0); > + const dateText = Services.intl.createDateTimeFormat(undefined, dateFormat).format(pingDate); > + const timeText = Services.intl.createDateTimeFormat(undefined, timeFormat).format(pingDate); > + let datetimeText = `${dateText} ${timeText}`; Explained in https://bugzilla.mozilla.org/show_bug.cgi?id=1396211#c6
Assignee | ||
Comment 15•7 years ago
|
||
Chris, would it be OK to change the date format in the ping name for en-US from 'dd/mm/yyyy hh:mm:ss' to 'dd/mm/yy h:mm:ss AM/PM'? That would get rid of the hardcoded space, and make the date/time properly internationalized.
Flags: needinfo?(chutten)
Comment 16•7 years ago
|
||
Oh, sorry for my review questions. I somehow skipped your responses. My responses below: > Because I'm trying to stay as close as possible to the current behavior, in case I'm not sure it would be my call to change the current time format. It seems to me that the author of the code just tried to get the date to display correctly, I doubt there's any deeper meaning behind enforcing hour12, but you can NI them here :) > But that would result in "9/3/17, 9:08 AM, PINGNAME" for en-US (double comma). Yeah, that's what you get for trying to use intl data with hardcoded semantics ;) I guess the double comma should be ok for en-US. > We need to show seconds, which would turn into a "10:40:00 AM" for en-US, with the header showing something as long as "10/10/2017 10:00:00 AM", and that requires to reduce the font size even further (15px, maybe 14px). that is the format that users expect in the locale. The only thing I can say is that if you try to ignore that, you're running into the risk of some other language using it anyway. If the UI is not ready for longer text, then it may not be ready for internationalized datetime formatting :( My general recommendation would be to go for dateStyle/timeStyle, use a single mozIntl with both of them, be ok with double comma, and make space for longer date/time string if you need to handle seconds. But that should be the call of the author of the code, so I'd suggest NI'ing them :)
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #13) > > dateWithoutTime is a timestamp (for the artifact above), today and yesterday are not > > Oh, then I'd recommend: > > ``` > const dateWithoutTime = new Date(pingDate); > dateWithoutTime.setHours(0, 0, 0, 0); > ``` I'm ready to throw a show at JavaScript… Looks like JS is comparing objects when you try to compare two dates let d1 = new Date('2017', '8', '3', '0', '0', '0', '0'); let d2 = new Date('2017', '8', '3', '0', '0', '0', '0'); print(d1 == d2); // false print(d1 === d2); // false print(d1.valueOf() === d2.valueOf()); //true print(d1.getTime() === d2.getTime()); //true
Comment 18•7 years ago
|
||
> print(d1.getTime() === d2.getTime()); //true
Use that.
Assignee | ||
Comment 19•7 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #18) > > print(d1.getTime() === d2.getTime()); //true > > Use that. That's what the current code is doing, just storing it into a variable to avoid calling .getTime() multiple times. Wouldn't it be less efficient to use .getTime directly in the if conditions?
Comment 20•7 years ago
|
||
> That's what the current code is doing, just storing it into a variable to avoid calling .getTime() multiple times.
> Wouldn't it be less efficient to use .getTime directly in the if conditions?
No, it's all good. I meant, use it instead of formatting it to a string to compare :)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(chutten)
Assignee | ||
Comment 22•7 years ago
|
||
Moving NI in a comment where it's clearer If we use properly internationalized dates: * The date appearing in the selector and header would change from 'dd/mm/yyyy hh:mm:ss' to 'dd/mm/yy h:mm:ss AM/PM'. * The text displayed in the Home panel will also change from 'dd/mm/yyyy hh:mm:ss, ping name' to 'dd/mm/yy, h:mm:ss AM/PM, ping name' (with a double comma). Are these changes OK? @zibi Isn't something like this supposed to work (according to bug 1329904)? Services.intl.createDateTimeFormat(undefined, {dateStyle: 'short', year: 'numeric'}).format(Date.now()) It still returns me 03/09/17, like "year" wasn't specified.
Flags: needinfo?(chutten)
Assignee | ||
Comment 23•7 years ago
|
||
Highlighted changes using en-US and Services.intl.createDateTimeFormat(undefined, {dateStyle: 'short', timeStyle: 'medium'}).format(pingDate) for the datetime. The CSS changes (incremented margin top, reduced font size) would not really be necessary, but I think they help (current layout is crowded in that area).
Comment 24•7 years ago
|
||
> Isn't something like this supposed to work (according to bug 1329904)? > Services.intl.createDateTimeFormat(undefined, {dateStyle: 'short', year: 'numeric'}).format(Date.now()) > It still returns me 03/09/17, like "year" wasn't specified. It's not supposed to work. When you select "dateStyle" or "timeStyle" you're basically telling the API to go look into OS Regional Preferences for the pattern that matches date-style or time-style you selected. We do this and use the result pattern which may not even have year (if the user customized it to not have the year token). Thus when you select dateStyle/timeStyle we ignore the token options as per http://searchfox.org/mozilla-central/source/js/src/builtin/Intl.js#2508
Comment 25•7 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #22) > Moving NI in a comment where it's clearer > > If we use properly internationalized dates: > * The date appearing in the selector and header would change from > 'dd/mm/yyyy hh:mm:ss' to 'dd/mm/yy h:mm:ss AM/PM'. > * The text displayed in the Home panel will also change from 'dd/mm/yyyy > hh:mm:ss, ping name' to 'dd/mm/yy, h:mm:ss AM/PM, ping name' (with a double > comma). > > Are these changes OK? If it is most convenient, then yes. I have no problems with double-comma or using user-specified date formats if this is what is recommended by l10n.
Flags: needinfo?(chutten)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•7 years ago
|
||
Comment on attachment 8903889 [details] Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code This should be in better shape for feedback now.
Attachment #8903889 -
Flags: feedback?(gandalf)
Comment 28•7 years ago
|
||
Comment on attachment 8903889 [details] Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code looks good to me! Thanks Flod! :)
Attachment #8903889 -
Flags: feedback?(gandalf) → feedback+
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → francesco.lodolo
Comment hidden (mozreview-request) |
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8903889 [details] Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code https://reviewboard.mozilla.org/r/175668/#review181452 Just a small nit from me. ::: toolkit/content/aboutTelemetry.css:97 (Diff revision 6) > > #controls { > display: flex; > - margin-top: 4px; > + margin-top: 6px; > justify-content: space-between; > + font-size: 16px; 16px is not in the Photon Design System typography recommendation: http://design.firefox.com/photon/visual/typography.html
Attachment #8903889 -
Flags: review?(chutten) → review-
Assignee | ||
Comment 31•7 years ago
|
||
(In reply to Chris H-C :chutten from comment #30) > 16px is not in the Photon Design System typography recommendation: > http://design.firefox.com/photon/visual/typography.html The current header is using 18px and weight normal(.heading). Would use 17px and weight 600 for both work? Looks like font sizes are all over the place at the moment: 15.95px (1.45rem) in the sidebar, 15px in tables (the only one actually in the Photon guidelines).
Comment 32•7 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #31) > (In reply to Chris H-C :chutten from comment #30) > > 16px is not in the Photon Design System typography recommendation: > > http://design.firefox.com/photon/visual/typography.html > > The current header is using 18px and weight normal(.heading). Would use 17px > and weight 600 for both work? > > Looks like font sizes are all over the place at the moment: 15.95px > (1.45rem) in the sidebar, 15px in tables (the only one actually in the > Photon guidelines). There are some bugs around exact sizing at the moment (most notably bug 1392532 on Linux), so not all of this may be deliberate... At any rate, I'm happy bringing things closer to the ideal. Even better is sourcing some classes directly from Common.css we could use instead. But 17px/600 ought to do the trick.
Assignee | ||
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8903889 [details] Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code https://reviewboard.mozilla.org/r/175668/#review181462 ::: toolkit/content/aboutTelemetry.css:97 (Diff revision 6) > > #controls { > display: flex; > - margin-top: 4px; > + margin-top: 6px; > justify-content: space-between; > + font-size: 16px; Setting .heading to 17px/600, and removing the increased margin since the font is smaller
Comment hidden (mozreview-request) |
Comment 35•7 years ago
|
||
mozreview-review |
Comment on attachment 8903889 [details] Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code https://reviewboard.mozilla.org/r/175668/#review181464 LGTM
Attachment #8903889 -
Flags: review?(chutten) → review+
Comment 36•7 years ago
|
||
Pushed by francesco.lodolo@mozillaitalia.org: https://hg.mozilla.org/integration/autoland/rev/78cce071b69e Use mozIntl for dates in aboutTelemetry.js, clean up unused code r=chutten
Comment 37•7 years ago
|
||
Backed out for eslint failure in toolkit/content/aboutTelemetry.js: Strings must use doublequote: https://hg.mozilla.org/integration/autoland/rev/0f5857f79e6f69ed8f25f1f7cd27ef28d7a2e541 Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=78cce071b69e26eb55df4da1f66409b3a2226d3b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=128640004&repo=autoland TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/content/aboutTelemetry.js:425:22 | Strings must use doublequote. (quotes) TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/content/aboutTelemetry.js:426:22 | Strings must use doublequote. (quotes)
Flags: needinfo?(francesco.lodolo)
Assignee | ||
Comment 38•7 years ago
|
||
Sorry about that, I never work with JS files and forgot about eslint :-\ Updating the patch after running tests locally $ ./mach lint -l eslint toolkit/content/aboutTelemetry.js ✖ 0 problems (0 errors, 0 warnings)
Flags: needinfo?(francesco.lodolo)
Comment hidden (mozreview-request) |
Comment 40•7 years ago
|
||
Pushed by francesco.lodolo@mozillaitalia.org: https://hg.mozilla.org/integration/autoland/rev/6e875574d0cb Use mozIntl for dates in aboutTelemetry.js, clean up unused code r=chutten
Comment 41•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6e875574d0cb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•