GNOME Bugzilla – Bug 763641
Multiselector failure while switching months
Last modified: 2017-04-17 18:20:40 UTC
When multi selecting months in month-view and switching months, the start date is in accordance with the position of the cell and not the date of the cell itself. This creates segmentation faults at times and also wrong selection of dates. Steps to Reproduce: -Enter month-view -Use shift and multiselect dates and go to the previous or next month using the keyboard moment keys. Actual Result: -The start date of the selection is in accordance with the position of the cell and is in current month. Expected Result: -The start date of the selection should be in accordance with the value of the start date and not its position.
Created attachment 324551 [details] [review] month-view: selection across months In this commit we change the data type of start_mark_cell and end_mark_cell to GDateTime from gint Currently, the mark_cell's are gint and while moving across months, they cause a problem as they don't take the new month into consideration but the cell value of the selector. To fix this, we changed the data type of the mark_cell's and modified all the functions dependant on it.
Review of attachment 324551 [details] [review]: Many memory leaks to fix. ::: src/gcal-month-view.c @@ +249,3 @@ priv->hovered_overflow_indicator = priv->pressed_overflow_indicator; + rebuild_popover_for_day (GCAL_MONTH_VIEW (widget), g_date_time_get_day_of_month(priv->end_mark_cell)); Add space between function and '(' @@ +255,3 @@ priv->pressed_overflow_indicator = -1; + priv->start_mark_cell = NULL; + priv->end_mark_cell = NULL; Two explicit memory leaks. Use g_clear_pointer() to properly unref the dates. @@ +262,2 @@ /* Swap dates if start > end */ + if (g_date_time_compare ((priv->start_mark_cell), (priv->end_mark_cell)) > 0) Remove the extra pairs of '()'. @@ +266,3 @@ + aux = (priv->start_mark_cell); + (priv->start_mark_cell) = (priv->end_mark_cell); + (priv->end_mark_cell) = aux; Ditto. @@ +271,2 @@ /* Only setup an end date when days are different */ + if (g_date_time_compare ((priv->start_mark_cell), (priv->end_mark_cell)) != 0) Ditto. @@ +273,2 @@ { + priv->end_mark_cell = g_date_time_add_days (priv->end_mark_cell, 1); Another explicit memory leak. It's loosing the reference to the previous end_mark_cell. Store in a local *aux variable and clear the previous date after. @@ +743,3 @@ if (selection) { + priv->end_mark_cell = g_date_time_new_local (priv->date->year, priv->date->month, present_day, 0, 0, 0); Jump lines in the arguments. @@ +822,3 @@ + priv->start_mark_cell = NULL; + priv->end_mark_cell = NULL; Again, more memory leaks. Use g_clear_pointer(). @@ +886,3 @@ priv->keyboard_cell = priv->days_delay + (priv->date->day - 1); + priv->start_mark_cell = NULL; + priv->end_mark_cell = NULL; Ditto. @@ +1401,2 @@ /* active cells */ + if (priv->start_mark_cell != NULL && priv->end_mark_cell != NULL) Simplify to 'if (priv->start_mark_cell && priv->end_mark_cell)' @@ +1666,2 @@ /* selection borders */ + if (priv->start_mark_cell != NULL && priv->end_mark_cell != NULL) Ditto. @@ +1858,2 @@ priv->keyboard_cell = clicked_cell; + priv->start_mark_cell = g_date_time_new_local (priv->date->year, priv->date->month, priv->keyboard_cell - priv->days_delay + 1, 0, 0, 0); Jump lines in the arguments. @@ +1893,3 @@ new_end_cell = real_cell (new_end_cell, priv->k); + if (priv->start_mark_cell != NULL) Simplify to 'if (priv->start_mark_cell)' @@ +1905,2 @@ priv->hovered_overflow_indicator = -1; + if ((g_date_time_get_day_of_month(priv->end_mark_cell) + priv->days_delay -1) != new_end_cell) Remove the extra pair of '()' @@ +1956,3 @@ { /* If the button is released over an invalid cell, entirely cancel the selection */ + priv->start_mark_cell = priv->end_mark_cell = NULL; Again, more memory leaks. Use g_clear_pointer(). @@ +2011,3 @@ + priv->start_mark_cell = NULL; + priv->end_mark_cell = NULL; Again, more memory leaks. Use g_clear_pointer(). @@ +2080,3 @@ + priv->start_mark_cell = NULL; + priv->end_mark_cell = NULL; Again, more memory leaks. Use g_clear_pointer(). Also, jump a line between these lines and the queue_draw().
Created attachment 324625 [details] [review] month-view: selection across months In this commit we change the data type of start_mark_cell and end_mark_cell to GDateTime from gint Currently, the mark_cell's are gint and while moving across months, they cause a problem as they don't take the new month into consideration but the cell value of the selector. To fix this, we changed the data type of the mark_cell's and modified all the functions dependant on it.
Review of attachment 324625 [details] [review]: Some work to do still. ::: src/gcal-month-view.c @@ +247,3 @@ return FALSE; + if (priv->pressed_overflow_indicator != -1 && g_date_time_compare (priv->start_mark_cell, priv->end_mark_cell) == 0 && Use g_date_time_equal() here. @@ +259,3 @@ + + g_clear_pointer (&priv->start_mark_cell, g_date_time_unref); + g_clear_pointer (&priv->end_mark_cell, g_date_time_unref); Since the patch heavily uses this code to cancel the selected range, it'd be better if you add a small function called 'cancel_selection (GcalMonthView *month_view)' with these 2 lines of code, and replace all the occurrencies by the function. @@ +275,2 @@ /* Only setup an end date when days are different */ + if (g_date_time_compare (priv->start_mark_cell, priv->end_mark_cell) != 0) You can use !g_date_time_equal() here. @@ +1437,3 @@ + lower_mark = 1; + else + lower_mark = icaltime_days_in_month (end_month, end_year); Simplify to 'lower_mark = start_year < end_year ? 1 : ...' and remove the pair of '{}'. @@ +1444,3 @@ + lower_mark = 1; + else + lower_mark = icaltime_days_in_month (end_month, end_year); Same here. @@ +1704,3 @@ + first_mark = real_cell (0, priv->k); + else + first_mark = real_cell (41, priv->k); Same here. @@ +1711,3 @@ + first_mark = real_cell (0, priv->k); + else + first_mark = real_cell (41, priv->k); Same here. @@ +1922,2 @@ priv->hovered_overflow_indicator = -1; + if ((g_date_time_get_day_of_month (priv->end_mark_cell) + priv->days_delay -1) != new_end_cell) Remove the extra '()'.
Created attachment 324660 [details] [review] month-view: selection across months In this commit we change the data type of start_mark_cell and end_mark_cell to GDateTime from gint Currently, the mark_cell's are gint and while moving across months, they cause a problem as they don't take the new month into consideration but the cell value of the selector. To fix this, we changed the data type of the mark_cell's and modified all the functions dependant on it.
Review of attachment 324660 [details] [review]: One more round and it's good to go. ::: src/gcal-month-view.c @@ +686,1 @@ } Remove the pair of '{}'. @@ +2151,3 @@ + g_clear_pointer (&priv->start_mark_cell, g_date_time_unref); + g_clear_pointer (&priv->end_mark_cell, g_date_time_unref); +} No newline at end of file Put this function in the top of the file.
Created attachment 324737 [details] [review] month-view: selection across months In this commit we change the data type of start_mark_cell and end_mark_cell to GDateTime from gint Currently, the mark_cell's are gint and while moving across months, they cause a problem as they don't take the new month into consideration but the cell value of the selector. To fix this, we changed the data type of the mark_cell's and modified all the functions dependant on it.
Review of attachment 324737 [details] [review]: Almost there. Apply the review below and, as discussed in IRC, don't draw past the first day or beyond the last day. ::: src/gcal-month-view.c @@ +676,3 @@ gboolean selection, valid_key; gdouble x, y; + gint min, max, diff, month_change, present_day; Rename 'present_day' to 'current_day'. @@ +1913,2 @@ priv->hovered_overflow_indicator = -1; + if (g_date_time_get_day_of_month (priv->end_mark_cell) + priv->days_delay -1 != new_end_cell) Add a space between '-' and '1'. @@ +1946,3 @@ GcalMonthViewPrivate *priv; gdouble x, y; + gint days, present_day; Rename 'present_day' to 'current_day'.
Created attachment 324809 [details] [review] month-view: selection across months In this commit we change the data type of start_mark_cell and end_mark_cell to GDateTime from gint Currently, the mark_cell's are gint and while moving across months, they cause a problem as they don't take the new month into consideration but the cell value of the selector. To fix this, we changed the data type of the mark_cell's and modified all the functions dependant on it.
Thanks for working on this issue. Attachment 324809 [details] pushed as a3cbaf1 - month-view: selection across months
Created attachment 326721 [details] calendar backtrace This should be reopened and reverted (for the stable branch). * it completely breaks mouse selection, resulting in crashes/segfaults * it makes 'other events' open the new event dialog, making it impossible to see the hidden events I don't quite get why new hardly-tested stuff gets added to the stable branch in the first place, thats not how the release schedule is supposed to work is it. Calendar 3.20.1 is more broken than 3.20.0.
Problems with mouse interaction. Needs to be fixed as soon as possible.
Created attachment 326872 [details] [review] month-view: Correcting mouse errors. In this commit, we remove the lines where we clear pointers start_dt and end_dt in show_popover() When we release the mouse select, there is a problem with the show_popover(). To fix this, we removed the lines where start_dt and end_dt are getting cleared.
Created attachment 328105 [details] [review] month-view: selection across months bug fix In this commit, we fix the errors occuring with the initial bug fix. Segmentation fault occurs at various places due to errors in initialization of certain variables. To fix this, we added a few initialization statements and checkers at every location of unref() and data access.
Review of attachment 328105 [details] [review]: ::: src/gcal-month-view.c @@ +204,3 @@ + if (priv->start_mark_cell) + g_clear_pointer (&priv->start_mark_cell, g_date_time_unref); g_clear_pointer() already does this NULL check. @@ +207,3 @@ + + if(priv->end_mark_cell) + g_clear_pointer (&priv->end_mark_cell, g_date_time_unref); Ditto. @@ +260,3 @@ ppriv = GCAL_SUBSCRIBER_VIEW (widget)->priv; + start_dt = g_date_time_add_days (priv->start_mark_cell, 0); + end_dt = g_date_time_add_days (priv->end_mark_cell, 0); There are inconsistencies in this section of the code: - It introduces these 2 new vars that should be unref() after use. However, they're only unref() in the else {} condition. - The function should either use ONLY priv->start/end_mark_cell or these 2 vars. It currently uses both in a mixed way. - There's no need to add the new vars here. We should track where the wrong unref() is happening instead. @@ +1925,3 @@ priv->keyboard_cell = new_end_cell; + if (priv->end_mark_cell && g_date_time_compare (priv->start_mark_cell, priv->end_mark_cell)) Should be priv->start_mark_cell. @@ +1928,2 @@ priv->hovered_overflow_indicator = -1; + if (priv->end_mark_cell && g_date_time_get_day_of_month (priv->end_mark_cell) + priv->days_delay - 1 != new_end_cell) I don't like this code. The issue is happening somewhere else.
Created attachment 328145 [details] [review] month-view: fix various reference issues with GDateTime The current code does multiple needless unrefs, uses the GDateTime API in a wrongly manner and leaks references all around. This causes the Calendar application to emit multiple warning messages and in the worst case, crash immediately. Fix that by rewriting the critical sections of the code that deals with the start and end mark cells.
Attachment 328145 [details] pushed as 692a963 - month-view: fix various reference issues with GDateTime