GNOME Bugzilla – Bug 778478
(Month view) sometimes events are folded to 'Other events' when not required
Last modified: 2017-06-23 07:21:50 UTC
Created attachment 345508 [details] Event folding issue (image) Some events are folded to 'Other events' popover even when the cell is empty. How to reproduce (this may depend on your screen size): 1. Create some fullday events in the following order: In May (or some other month) 2017, a. event 1: from March 1 to May 3 b. event 2: from March 2 to May 4 c. event 3: from March 2 to May 4 d. event 4: from March 2 to May 4 Add more from March 2 to May 4 if required. Result: The item in March 1 is not shown in the cell. It is shown in 'Other events'. Please see the attached screenshot. Thanks
Created attachment 350710 [details] [review] Fix: The overflow layout used to display "Other X events" text when there is no enough space to accomodate more events. But due to the way the events were sorted, some events are folded to 'Other events' popover even when the cell is empty. This bug is only visible when all the events are added at once(i.e at same session). But goes away when calendar is restarted. Since there is no such sorting when calendar is restarted(started) to sort those events, we can also eliminate such sorting when events are added, atleast to make them consistent. https://bugzilla.gnome.org/show_bug.cgi?id=778478
Review of attachment 350710 [details] [review]: I don't think this is the correct solution. The problem here is that gcal_event_widget_compare_by_length() isn't doing the right thing. When the lengths are equal, the events should be sorted according to their start date. Fixing that function would be the correct solution. Beside that, while the body of your commit message is good, the title of the commit isn't quite right. Could you please follow the guidelines in https://wiki.gnome.org/Newcomers/SubmitPatch ? Thanks for the patch!
Created attachment 352992 [details] [review] rework/add new event's sort function. But due to the way the events were sorted, some events are folded to 'Other events' popover even when the cell has enough space to accomodate the event or when it is not required to be folded. We rework how the events will be sorted. Firstly, the events that has the closest start date should come first, then if two or more events have same start date then the longest should come first. And if the still they have same length, the last one(s) that is/are added should not be folded.
Review of attachment 352992 [details] [review]: Needs some work. ::: src/gcal-event-widget.c @@ +1221,3 @@ + + return diff; +} First, this whole function is not following the GNU C Coding Style. See https://developer.gnome.org/programming-guidelines/stable/c-coding-style.html.en for reference. The second 'if' should be "if (diff != 0)\n return diff;" followed by "return 0;"
Created attachment 353156 [details] [review] [PATCH V2] rework/add new event's sort function.
Created attachment 353434 [details] [review] [PATCH V3] rework/add new event's sort function. reformat code
Created attachment 353526 [details] [review] [PATCH V4] rework/add new event's sort function. The patch introduce a new sort function that sort events first by start date(the closest one first). And if two or more events have same start date then it sort then according to their length(the longest one first). And lastly if still they have same start date and length, then it sort them according to the `last_modified` date/time.
Created attachment 353682 [details] [review] [PATCH V5] rework/add new event's sort function.
Review of attachment 353682 [details] [review]: The commit title prefix should be "event-widget" only. The commit message looks good! Some comments below. Overall, I think this patch idea is correct, let's just fix the style issues. ::: src/gcal-event-widget.c @@ +1249,3 @@ + diff = gcal_event_widget_compare_by_start_date (widget1, widget2); + + if(diff != 0) Style issue (missing space after 'if') @@ +1254,3 @@ + diff = gcal_event_widget_compare_by_length (widget1, widget2); + + if(diff != 0) Style issue (missing space after 'if') @@ +1259,3 @@ + ECalComponentDateTime ecal_comp_dt; + + e_cal_component_get_last_modified (gcal_event_get_component(widget1->event), &ecal_comp_dt.value); - Style issue (missing space before parenthesis) - You could declare an "icaltimetype" variable, rather than an ECalComponentDateTime @@ +1262,3 @@ + GDateTime* dt_time1 = icaltime_to_datetime (ecal_comp_dt.value); + + e_cal_component_get_last_modified (gcal_event_get_component(widget2->event), &ecal_comp_dt.value); - Style issue (missing space before parenthesis) - You could declare an "icaltimetype" variable, rather than an ECalComponentDateTime @@ +1263,3 @@ + + e_cal_component_get_last_modified (gcal_event_get_component(widget2->event), &ecal_comp_dt.value); + GDateTime* dt_time2 = icaltime_to_datetime (ecal_comp_dt.value); Don't declare variables in the middle of the function. All variables should be declared at the start of the function. ::: src/gcal-event-widget.h @@ +72,3 @@ +gint gcal_event_widget_sort_events (GcalEventWidget *widget1, + GcalEventWidget *widget2); Style issue: align parameters with functions above.
Created attachment 354011 [details] [review] [PATCH V6] rework/add new event's sort function. Fix style issues. ~I need to be more careful next-time~
Comment on attachment 354011 [details] [review] [PATCH V6] rework/add new event's sort function. The patch leaks the two GDateTimes, and still has some style issues (namely, all variable should've been declared at the top of the function).
Created attachment 354297 [details] [review] event-widget : Add/rework new event sort function Fix those two comments.
Thanks for the patch! Attachment 354297 [details] pushed as 0a02a12 - event-widget : Add/rework new event sort function