GNOME Bugzilla – Bug 777416
(Week view) different events may get joined into one
Last modified: 2017-07-11 01:47:26 UTC
Created attachment 343688 [details] week-view event creation (video) Two separate events may be shown as joint in week-view How to reproduce: It's a bit hard explaining in words. Please see the attached screencast. Please see event 'three' and 'four' in the screencast. Thanks
Created attachment 350701 [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
Sorry this patch https://bugzilla.gnome.org/attachment.cgi?id=350701&action=diff belong to this bug https://bugzilla.gnome.org/show_bug.cgi?id=778478.
Created attachment 350711 [details] Propose Patch for "different events may get joined into one" The events only appears to be jointed if the calendar color does not have some transparency. As you can see from the attachment the events are distinguishable when the color have some transparency. The only fix i see here is to make all the calendar colors have some transparency.
(In reply to Abdullahi Usman from comment #3) > The only fix i see here is to make all the calendar colors have some > transparency. The "only fix" would be fixing the algorithm that positions the events around the week grid. It's available in src/views/gcal-week-grid.c.
Created attachment 354540 [details] [review] seperate week's events horizontal and vertical overlap. This patch tries to give week's events proper width by taking into consideration all the events that are within same time range/span. It also tries to position events into right index by checking into all the indexes to get an empty index.
Review of attachment 354540 [details] [review]: I'm impressed! Thanks for tracking this hairy issue down. Some comments regarding the code below, but overall, you really found the heart of the issue. About the commit message: - Reduce the commit title (the maximum allowed is 72 characters) - "Week view" rather than "WEEK-VIEW" - Capitalize 'if' at the last paragraph ::: src/views/gcal-week-grid.c @@ +212,3 @@ + if (idx == GPOINTER_TO_INT (g_ptr_array_index (array, ii))) + idx++; + } I'm almost sure this can be improved… This turns the function into a quadratic complexity one. @@ +668,3 @@ + + } + It's better if we only fix "count_overlaps_at_range()" rather than adding this other loop here.
Created attachment 354808 [details] [review] seperate week's events overlaps This patch tries to handle things differently than before. First of all it fixes int16_compare(), which before does not sort events into order. And secondly trying to find the overlaps at just the range where the events leads to overlap if there are other events at same ranges which are have long ranges and also span ranges again with other events. So now the patch tries to find the number of all of the events, and thereby find the range with the highest number of events. The overlap at this range would the overlap for all these events at these ranges. I dont know if this is still correct. or if there is a better way to handle this.
Created attachment 354856 [details] [review] seperate week's events overlaps
Review of attachment 354856 [details] [review]: This patch works very well, and the commit message is excellent too. Some comments below - notice that the uint16_compare() function is kinda wrong, but not in the way you described. Please update the commit message to reflect that. Almost there! ::: src/views/gcal-week-grid.c @@ +188,3 @@ gconstpointer b) { + return GPOINTER_TO_INT (*(gint*)a) - GPOINTER_TO_INT (*(gint*)b); I don't think this is right. I realize now that we should be using GPOINTER_TO_UINT() rather than TO_INT(), but looks like this is trying to access the memory? We're not storing an actual pointer to some memory, but a number - and just pretending the number is a pointer. @@ +259,3 @@ + g_return_val_if_fail (event_start <= day_end, 0); + g_return_val_if_fail (event_end <= day_end, 0); + g_return_val_if_fail (event_end >= day_start, 0); For the sake of performance, let's just not make these checks.
Created attachment 355027 [details] int16_compare() bug Thanks you. Regarding g_ptr_array_sort, according to https://developer.gnome.org/glib/stable/glib-Pointer-Arrays.html#g-ptr-array-sort, it says that "Note that the comparison function for g_ptr_array_sort() doesn't take the pointers from the array as arguments, it takes pointers to the pointers in the array. ". So to my own understanding the (*(gint*)a) should only dereference the pointer to the pointer not the pointer(in our case the integer) we store, while GPOINTER_TO_UINT() gets our stored value. I was able to produce the attach bug which is similar to the already reported bug, and when i changed it to (*(gint*)x), i get the expected result. And also i use GUINT_TO_POINTER() to store the value.
Created attachment 355255 [details] [review] seperate week's events overlaps This patch removes some checks and replace INT_TO_POINTER() and POINTER_TO_INT() with UINT_TO_POINTER() and POINTER_TO_UINT(). It also rename int16_compare() to uint16_compare(). As i have save with this attachment https://bugzilla.gnome.org/attachment.cgi?id=355027 and in this comment https://bugzilla.gnome.org/show_bug.cgi?id=777416#c10 , the pointer from g_array_sort() needs to be dereference without that we are back to same old bug. But if there is another way to do that or i may have misunderstood, please let me know. Thanks.
Review of attachment 355255 [details] [review]: Looks good ::: src/views/gcal-week-grid.c @@ +257,3 @@ + g_return_val_if_fail (day_start < day_end, 0); + g_return_val_if_fail (event_start <= event_end, 0); + g_return_val_if_fail (event_start >= day_start, 0); Nitpick: remove these assertions (they may hurt performance)
Created attachment 355304 [details] [review] week-grid : seperate week's events overlaps. Remove the needless code.
Thanks for your hard work! It's really appreciated. This patch is also present in gnome-3-24 branch, and will be released with the next 3.24 stable release. Attachment 355304 [details] pushed as 1360fe0 and 9f4bc90 - week-grid : seperate week's events overlaps.