-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(agenda): all day event offsets, locale display #3695
base: master
Are you sure you want to change the base?
Conversation
79c204f
to
8148008
Compare
@glemco it seems like you're the de facto owner of the agenda app, so mentioning you here :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking care of this! I remember testing this across time-zones was quite convoluted and I didn't know about this Contract.
Added some minor comments but it looks good to me.
apps/agenda/agenda.js
Outdated
// All day events end at the midnight boundary of the following day. Here, we | ||
// subtract one second for all day events so the days display correctly. | ||
var end = getDate((+ev.timestamp) + (+ev.durationInSeconds) - ev.allDay, ev.allDay); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this looks clever, requires the reader to check your comment to understand why you'd subtract what's basically a boolean value from a timestamp. I'd be a bit more verbose just for the sake of clarity, what do you think?
// All day events end at the midnight boundary of the following day. Here, we | |
// subtract one second for all day events so the days display correctly. | |
var end = getDate((+ev.timestamp) + (+ev.durationInSeconds) - ev.allDay, ev.allDay); | |
// All day events end at the midnight boundary of the following day. Here, we | |
// subtract one second for all day events so the days display correctly. | |
const allDayOffset = ev.allDay ? 1 : 0; | |
var end = getDate((+ev.timestamp) + (+ev.durationInSeconds) - allDayOffset, ev.allDay); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hilariously, that's exactly what I had wanted to do, but decided to be "clever" since it's an embedded device with minimal resources, etc (in the name of efficiency). Even told the wife how I already hated what I'd done :)
Every time I do something "clever", I regret it, so I'm glad we agree! lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid colliding with the "offset" concept in getDate()
, I chose a slightly different name than you did above. Let me know if this is alright with you.
if(!includeDay && ev.allDay) { | ||
//single day all day |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, that was actually not too readable and I got myself confused.. I wonder if we should change it to something like sameDay
, that'd probably be more meaningful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this should be renamed, but sameDay
would reverse the current boolean's values, and that variable is used in quite a few places that would have to be fixed.
I was already planning on doing a second pass with some cleanup like this to help make the code a little more clear, so I'll take care of the variable renaming then to avoid having too many changes in this PR, which might make it harder to understand.
8148008
to
e315cac
Compare
Currently, allDay events are off by one day for negative timezones. Per the CalendarContract: If allDay is set to 1 eventTimezone must be "UTC" and the time must correspond to a midnight boundary. For example, in GMT-2:00, an all day event on December 2nd (beginning at 00:00:00) will be wrongly displayed as starting on December 1st, since the locale will determine that the event's start time is actually 22:00:00 on Dec 1. Source: https://developer.android.com/reference/android/provider/CalendarContract.Events.html This commit: * Corrects the offset back to UTC 00:00:00 for allDay events * Fixes the conditional for single-day all day events in showEvent() * Fixes the display of formatDateShort() for some English locales by also removing any trailing commas or whitespace when the year is removed
e315cac
to
2d4228f
Compare
Should be ready for a final review. Thanks! |
Currently, allDay events are off by one day for negative timezones. Per the CalendarContract:
For example, in GMT-2:00, an all day event on December 2nd (beginning at 00:00:00) will be wrongly displayed as starting on December 1st, since the locale will determine that the event's start time is actually 22:00:00 on Dec 1.
Source:
https://developer.android.com/reference/android/provider/CalendarContract.Events.html
This commit:
Corrects the offset back to UTC 00:00:00 for allDay events
Fixes the conditional for single-day all day events in showEvent()
Fixes the display of formatDateShort() for some English locales by also removing any trailing commas or whitespace when the year is removed