Closed
Bug 987427
Opened 10 years ago
Closed 10 years ago
[B2G][Calendar] Notifications for calendar alarms set for 2 hours before the event are incorrect.
Categories
(Firefox OS Graveyard :: Gaia::Calendar, defect)
Tracking
(tracking-b2g:backlog, b2g-v1.3 affected, b2g-v1.3T affected, b2g-v1.4 affected)
People
(Reporter: demerick, Assigned: mmedeiros)
References
Details
(Whiteboard: Permafail, [priority][p=3])
Attachments
(3 files)
Description: If the user has an alarm set for 2 hours before an event the notification will incorrectly state 'starting in an hour'. Repro Steps: 1) Update a Buri to BuildID: 20140324000202 2) Open Calendar app from the home screen 3) Tap '+' in the upper right hand corner 4) Set the start time to 2 hours and 5 minutes from the current time 5) Make sure the end time is after the start time 6) Set a reminder alarm for '2 hours before' 7) Exit the calendar app and wait 5 minutes 8) View the notification Actual: The notification incorrectly states 'starting in an hour'. Expected: The notification should say 'starting in 2 hours'. 1.4 Environmental Variables: Device: Buri 1.4 MOZ BuildID: 20140324000202 Gaia: 730670951e40b2317a167fcd07c398bb662d6e87 Gecko: a44f8b39c2c8 Version: 30.0a2 Firmware Version: v1.2-device.cfg Repro frequency: 100% Link to failed test case: https://moztrap.mozilla.org/manage/case/7500/ See attached: logcat, screenshot
This issue also occurs on the Buri v 1.3.0 Mozilla RIL 1.3 Environmental Variables: Device: Buri 1.3 MOZ BuildID: 20140324004001 Gaia: f7742fb4929cc57c9f72955ce5cebb8279745ac0 Gecko: e42b778a010f Version: 28.0 Firmware Version: v1.2-device.cfg The notification for an alarm set for 2 hours before the event states 'starting in an hour'.
Updated•10 years ago
|
status-b2g-v1.3T:
--- → affected
Whiteboard: burirun1.4-2 → burirun1.4-2, 1.3tarakorun2
Comment 2•10 years ago
|
||
can we get triage to look at this? displaying wrong information in notifications needs fixing.
blocking-b2g: --- → 1.4?
Updated•10 years ago
|
blocking-b2g: 1.4? → 1.4+
Comment 3•10 years ago
|
||
Thanks, Dylan! By the way, is it possible to find someone to look at this issue? Thank you.
Flags: needinfo?(doliver)
Comment 4•10 years ago
|
||
Is it possible if we move this bug to 2.0?
Assignee | ||
Comment 6•10 years ago
|
||
I'll take a look at this issue to understand better what is going on, just need to finish a couple tasks before. My bet (without looking at the code or trying to reproduce it) is that we might have some "rounding" issue (alarm triggered 1h59m50s before event rounded down to 1h). If anyone else have any "insight" of other potential causes, please let me know.
Flags: needinfo?(doliver) → needinfo?(mmedeiros)
Assignee | ||
Comment 7•10 years ago
|
||
confirmed that it's a rounding issue. Any value smaller than 2 hours returns "in an hour"... and you guys wont believe on the difference that is causing the rounding error. `Date.now()` returned `1397684100109` and the event start was `1397691300000`, the difference between these values is `1.9999697222222221` hours. var df = navigator.mozL10n.DateTimeFormat(); df.fromNow(new Date(Date.now() + 1.99 * 60 * 60 * 1000)) === "in an hour" now we have a "big" dilemma, should we "fix" the gaia/shared/js/l10n_date.js (round difference to closest minute) or should we simply do a quick hack on calendar (controllers/alarm.js)? the hack on calendar can be as simple as adding `begins.setSeconds(25)` - 25 seconds is more than enough to avoid rounding issues, difference is usually less than 1 second.
Flags: needinfo?(mmedeiros)
Comment 8•10 years ago
|
||
(In reply to Kevin Hu [:khu] from comment #4) > Is it possible if we move this bug to 2.0? Agreed that we won't block 1.4 for it so 1.4+ was the wrong call. Marking it priority for a quick fix. The rounding bug happens with all of the time options but it's clearly worst if you choose the 2 hour reminder as the resulting "one hour" notification text is just wrong. Other options are less severe in terms of how far off we are -- a 1 hour reminder becomes "starting in 59 minutes" and a 1 day reminder is "starting in 23 hours".
blocking-b2g: 1.4? → backlog
Whiteboard: burirun1.4-2, 1.3tarakorun2 → burirun1.4-2, 1.3tarakorun2, [priority]
Comment 9•10 years ago
|
||
Thank you, Dylan! This is super helpful.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mmedeiros
Whiteboard: burirun1.4-2, 1.3tarakorun2, [priority] → burirun1.4-2, 1.3tarakorun2, [priority][p=3]
Target Milestone: --- → 1.4 S6 (25apr)
Assignee | ||
Comment 10•10 years ago
|
||
quick hack on calendar app that fixes the problem (update seconds and milliseconds to match current time).. unsure if we should change the l10n lib instead.
Attachment #8410780 -
Flags: review?(gaye)
Comment 11•10 years ago
|
||
Comment on attachment 8410780 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18561 After lots of being unsure, I am ok with this. Thanks Miller!
Attachment #8410780 -
Flags: review?(gaye) → review+
Comment 12•10 years ago
|
||
Let's also go ahead and suggest that the shared date code do something smarter.
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8410780 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18561 Gareth, sorry for the extra work but based on your comments I decided to change the logic and "fix" the l10n_date code instead. If the hack needed that much explanation it means it wasn't a simple "fix"...
Attachment #8410780 -
Flags: review+ → review?(gaye)
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8410780 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18561 Fabien, can you please review my changes? I initially did a quick hack on the calendar app itself but I feel this logic should be handled by the l10n_date instead. It will probably avoid headaches. Thanks.
Attachment #8410780 -
Flags: review?(fabien)
Comment 16•10 years ago
|
||
Comment on attachment 8410780 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18561 Calendar part/test still looks great.
Attachment #8410780 -
Flags: review?(gaye) → review+
Updated•10 years ago
|
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8410780 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18561 Hi :stas, I need your review on this patch. Thanks!
Attachment #8410780 -
Flags: review?(fabien) → review?(stas)
Comment 18•10 years ago
|
||
Comment on attachment 8410780 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18561 r=me with a comment that I'd like to see addressed. l10n_date.js might be used by third-party app developers to create localized cross-browser webapps. For instance, it is mentioned on https://developer.mozilla.org/en-US/Apps/Build/Localization/Localizing_Firefox_OS_Apps#l10n_date_Script. As much as I like them and find them useful, I'd prefer to avoid using non-ES5 methods like Math.sign. Maybe re-write this code like this? if (Math.abs(secDiff) > 60) { secDiff = secDiff > 0 ? Math.ceil(secDiff) : Math.floor(secDiff); } Also, it looks like the test doesn't have to be in an else block and absDiff could be calculated closer to where it's used.
Attachment #8410780 -
Flags: review?(stas) → review+
Updated•10 years ago
|
Whiteboard: burirun1.4-2, 1.3tarakorun2, [priority][p=3] → Permafail, [priority][p=3]
Assignee | ||
Comment 19•10 years ago
|
||
landed into master: https://github.com/mozilla-b2g/gaia/commit/badc73ee7f108fa631150bded0cc57e92aad810e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 20•10 years ago
|
||
[Environment] Gaia c1a8cbaac1d921cfb50e3a2600720b75cf5afabd Gecko https://hg.mozilla.org/mozilla-central/rev/20ca234fd62b BuildID 20140511160202 Version 32.0a1 ro.build.version.incremental=324 ro.build.date=Thu Dec 19 14:04:55 CST 2013 [Result] PASS
Status: RESOLVED → VERIFIED
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•