GNOME Bugzilla – Bug 763015
[year view] criticals on end of the month selection
Last modified: 2017-04-17 18:20:40 UTC
Created attachment 322924 [details] [review] year view critical fix I noticed when selecting the last day of the month with events on them resulting in some glib criticals on the console. Turns out there's some code in the event sidebar relying on adding +1 to the day while creating a g_date_time, resulting in an invalid datetime for the last day of the month. Use g_date_time_add_days instead.
Created attachment 322970 [details] [review] year view critical fix Noticed this was done at a second spot as well. This one was only triggered when selecting a date range that ended at the last day of a month and then adding an event.
Created attachment 323194 [details] [review] multiday selection fix Noticed the same was happening for final-day-of-month multi-day events.
Review of attachment 322970 [details] [review]: LGTM.
Review of attachment 323194 [details] [review]: This patch does not apply cleanly on master. Also, I still see a issue when selecting a date range that ends in the last day of month (it shows me, e.g. April 00).
Created attachment 323700 [details] [review] month-view: fix end-of-month multi-day selections Use g_date_time_add_days to add a day to the final day of the month to fix behaviour when selecting a range up to the last day of the month. Also, fix the datetime comparison: anything above 0 indicates a multi-day event.
Created attachment 323701 [details] [review] month-view: fix date adjust mishap
Created attachment 323702 [details] [review] quick-add-popover: adjust end date properly Subtracting 1 from the first day of the month leads to an invalid date being shown: use a proper function to adjust the date.
(In reply to Georges Basile Stavracas Neto from comment #4) > Review of attachment 323194 [details] [review] [review]: > > This patch does not apply cleanly on master. Also, I still see a issue when > selecting a date range that ends in the last day of month (it shows me, e.g. > April 00). updated to head Nice catch on the displayed day, it gets fixed in the attachment in comment #7 (note that the actual dates used for the event were ok). Comment #6 contains another day adjustment fix, however this code (rebuild_popover_for_day) afaics never gets triggered and might need further cleanup.
Review of attachment 323700 [details] [review]: Remove the extra line in quick-add-popover and it'll be OK. ::: src/gcal-quick-add-popover.c @@ +386,3 @@ * Everything else starts now and lasts 1 hour. */ + all_day = datetime_compare_date (self->date_end, self->date_start) > 0; The purpose of this line is that, when we select 1 day only, the event is created with the local time. Only if the user selects 2+ days, we consider the event all-day.
Review of attachment 323701 [details] [review]: LGTM.
Review of attachment 323702 [details] [review]: Nice catch, thanks.
Created attachment 323787 [details] [review] month-view: fix end-of-month multi-day selections Use g_date_time_add_days to add a day to the final day of the month to fix behaviour when selecting a range up to the last day of the month. Also, fix the datetime comparison: anything above 0 indicates a multi-day event.
Created attachment 323788 [details] [review] month-view: fix end-of-month multi-day selections Use g_date_time_add_days to add a day to the final day of the month to fix behaviour when selecting a range up to the last day of the month.
Review of attachment 323788 [details] [review]: One more nitpick, probably the last one. ::: src/gcal-month-view.c @@ +263,3 @@ else { + GDateTime *start_dt, *tmp_dt, *end_dt; Put the tmp_dt variable (and all the code related to it) inside the 'if (start != end)' condition. It is not used outside that context.
Review of attachment 323701 [details] [review]: Just noticed a minor style issue. See below. ::: src/gcal-month-view.c @@ +518,3 @@ 0, 0, 0); + dt_end = g_date_time_add_days(dt_start, 1); Put a space between the function name and '('.
Review of attachment 323702 [details] [review]: Another style issue. ::: src/gcal-quick-add-popover.c @@ +233,2 @@ if (dtend->is_date) + icaltime_adjust(dtend, -1, 0, 0, 0); Put a space between the function name and '('.
Created attachment 323795 [details] [review] year-view: criticals on last day of month events Adding 1 to the day variable at the end of the month results in an invalid datetime, use g_date_time_add_days instead.
Created attachment 323796 [details] [review] month-view: fix end-of-month multi-day selections Use g_date_time_add_days to add a day to the final day of the month to fix behaviour when selecting a range up to the last day of the month.
Created attachment 323797 [details] [review] month-view: fix date adjust mishap
Created attachment 323798 [details] [review] quick-add-popover: adjust end date properly Subtracting 1 from the first day of the month leads to an invalid date being shown: use a proper function to adjust the date.
Review of attachment 323796 [details] [review]: A minor nitpick. ::: src/gcal-month-view.c @@ +281,2 @@ /* Only setup an end date when days are different */ + if (start_day != end_day) { Use the GNU C style for the curly braces.
Review of attachment 323797 [details] [review]: LGTM.
Review of attachment 323798 [details] [review]: LGTM.
Review of attachment 323795 [details] [review]: Looks good, but can be simplified. ::: src/gcal-year-view.c @@ +290,3 @@ + g_date_time_get_month (date), + g_date_time_get_day_of_month (date), + 0, 0, 0); The tmp_date is not needed here. Use 'second_date = g_date_time_add_days (date, 1)' directly. @@ +1064,2 @@ start_date = g_date_time_new_local (dtstart->year, dtstart->month, dtstart->day, 0, 0, 0); + tmp_date = g_date_time_new_local (dtend->year, dtend->month, dtend->day, 0, 0, 0); The tmp_date var is not needed here. Use end_date directly.
Created attachment 323909 [details] [review] month-view: fix end-of-month multi-day selections Use g_date_time_add_days to add a day to the final day of the month to fix behaviour when selecting a range up to the last day of the month.
Review of attachment 323909 [details] [review]: Another nitpick. ::: src/gcal-month-view.c @@ +287,3 @@ + end_dt = g_date_time_add_days (tmp_dt, 1); + + g_clear_pointer (&tmp_dt, g_date_time_unref); Fix code alignment inside the 'if' condition.
Created attachment 323913 [details] [review] month-view: fix end-of-month multi-day selections Use g_date_time_add_days to add a day to the final day of the month to fix behaviour when selecting a range up to the last day of the month.
Created attachment 323914 [details] [review] year-view: criticals on last day of month events Adding 1 to the day variable at the end of the month results in an invalid datetime, use g_date_time_add_days instead.
Thanks for your hard work on this bug. Attachment 323797 [details] pushed as c67d520 - month-view: fix date adjust mishap Attachment 323798 [details] pushed as 2ac228a - quick-add-popover: adjust end date properly Attachment 323913 [details] pushed as 96d2b37 - month-view: fix end-of-month multi-day selections Attachment 323914 [details] pushed as 465d786 - year-view: criticals on last day of month events