[california] Don't persist mutating Events in Gee.TreeSet: Bug #732671

[Jim Nelson <jnelson src gnome org>]

commit 75d1f4715f987137b792dad0a811fb8a09bdf825
Author: Jim Nelson <jim yorba org>
Date:   Wed Sep 10 15:52:53 2014 -0700

    Don't persist mutating Events in Gee.TreeSet: Bug #732671
    
    Component.Event's properties mutate, which can cause cogency problems
    in Gee.TreeSet's red-black tree that cannot be solved externally.
    This change stores Events in HashSets (which use an equality function
    which does not deal with mutable properties) and sorts them only when
    needed.
    
    See also bug #736444.

 src/view/common/common-events-cell.vala |   40 +++++++++++++++----------
 src/view/week/week-day-pane.vala        |   49 ++++++++++++++++++++-----------
 2 files changed, 56 insertions(+), 33 deletions(-)
---
diff --git a/src/view/common/common-events-cell.vala b/src/view/common/common-events-cell.vala
index 0e4ec52..912aa4c 100644
--- a/src/view/common/common-events-cell.vala
+++ b/src/view/common/common-events-cell.vala
@@ -85,7 +85,7 @@ internal abstract class EventsCell : Gtk.EventBox, InstanceContainer {
     /**
      * @inheritDoc
      */
-    public int event_count { get { return sorted_events.size; } }
+    public int event_count { get { return days_events.size; } }
     
     /**
      * @inheritDoc
@@ -99,7 +99,7 @@ internal abstract class EventsCell : Gtk.EventBox, InstanceContainer {
      */
     public View.Palette palette { get; private set; }
     
-    private Gee.TreeSet<Component.Event> sorted_events = new 
Gee.TreeSet<Component.Event>(all_day_comparator);
+    private Gee.HashSet<Component.Event> days_events = new Gee.HashSet<Component.Event>();
     private Gee.HashMap<int, Component.Event> line_to_event = new Gee.HashMap<int, Component.Event>();
     private Gtk.DrawingArea canvas = new Gtk.DrawingArea();
     
@@ -216,15 +216,19 @@ internal abstract class EventsCell : Gtk.EventBox, InstanceContainer {
     public void clear_events() {
         line_to_event.clear();
         
-        foreach (Component.Event event in sorted_events.to_array())
+        foreach (Component.Event event in days_events.to_array())
             internal_remove_event(event);
         
         queue_draw();
     }
     
     public void add_event(Component.Event event) {
-        if (!sorted_events.add(event))
+        if (!days_events.add(event)) {
+            debug("Unable to add event %s to cell for %s: already present", event.to_string(),
+                date.to_string());
+            
             return;
+        }
         
         // subscribe to interesting mutable properties
         event.notify[Component.Event.PROP_SUMMARY].connect(queue_draw);
@@ -237,8 +241,12 @@ internal abstract class EventsCell : Gtk.EventBox, InstanceContainer {
     }
     
     private bool internal_remove_event(Component.Event event) {
-        if (!sorted_events.remove(event))
+        if (!days_events.remove(event)) {
+            debug("Unable to remove event %s from cell for %s: not present in sorted_events",
+                event.to_string(), date.to_string());
+            
             return false;
+        }
         
         event.notify[Component.Event.PROP_SUMMARY].disconnect(queue_draw);
         event.notify[Component.Event.PROP_DATE_SPAN].disconnect(on_span_updated);
@@ -263,7 +271,7 @@ internal abstract class EventsCell : Gtk.EventBox, InstanceContainer {
      * calendar in question has any events in this date.
      */
     public void notify_calendar_visibility_changed(Backing.CalendarSource calendar_source) {
-        if (!traverse<Component.Event>(sorted_events).any((event) => event.calendar_source == 
calendar_source))
+        if (!traverse<Component.Event>(days_events).any((event) => event.calendar_source == calendar_source))
             return;
         
         // found one
@@ -274,7 +282,7 @@ internal abstract class EventsCell : Gtk.EventBox, InstanceContainer {
     // Called internally by other Cells when (a) they're in charge of assigning a multi-day event
     // its line number for the week and (b) that line number has changed.
     private void notify_assigned_line_number_changed(Gee.Collection<Component.Event> events) {
-        if (!traverse<Component.Event>(sorted_events).contains_any(events))
+        if (!traverse<Component.Event>(days_events).contains_any(events))
             return;
         
         assign_line_numbers();
@@ -294,10 +302,12 @@ internal abstract class EventsCell : Gtk.EventBox, InstanceContainer {
         // reassigned because of this
         Gee.ArrayList<Component.Event> reassigned = new Gee.ArrayList<Component.Event>();
         
+        // Can't persist events in TreeSet because mutation is not handled well, see
+        // https://bugzilla.gnome.org/show_bug.cgi?id=736444
+        Gee.TreeSet<Component.Event> sorted_events = traverse<Component.Event>(days_events)
+            .filter(event => event.calendar_source.visible)
+            .to_tree_set(all_day_comparator);
         foreach (Component.Event event in sorted_events) {
-            if (!event.calendar_source.visible)
-                continue;
-            
             bool notify_reassigned = false;
             if (event.is_day_spanning) {
                 // get the first day of this week the event exists in ... if not the current cell's
@@ -382,14 +392,12 @@ internal abstract class EventsCell : Gtk.EventBox, InstanceContainer {
     private void on_span_updated(Object object, ParamSpec param) {
         Component.Event event = (Component.Event) object;
         
-        // remove from cell if no longer in this day, otherwise remove and add again to sorted_events
-        // to re-sort
-        if (!(date in event.get_event_date_span(Calendar.Timezone.local))) {
+        // remove from cell if no longer in this day, otherwise re-assign line numbers
+        // due to date/time change
+        if (!(date in event.get_event_date_span(Calendar.Timezone.local)))
             remove_event(event);
-        } else if (sorted_events.remove(event)) {
-            sorted_events.add(event);
+        else
             assign_line_numbers();
-        }
         
         queue_draw();
     }
diff --git a/src/view/week/week-day-pane.vala b/src/view/week/week-day-pane.vala
index 0298618..02c33c9 100644
--- a/src/view/week/week-day-pane.vala
+++ b/src/view/week/week-day-pane.vala
@@ -52,7 +52,7 @@ internal class DayPane : Pane, Common.InstanceContainer {
      */
     public Calendar.Span contained_span { get { return date; } }
     
-    private Gee.TreeSet<Component.Event> days_events = new Gee.TreeSet<Component.Event>();
+    private Gee.HashSet<Component.Event> days_events = new Gee.HashSet<Component.Event>();
     private Scheduled? scheduled_monitor = null;
     
     public DayPane(Grid owner, Calendar.Date date) {
@@ -107,8 +107,12 @@ internal class DayPane : Pane, Common.InstanceContainer {
     }
     
     public void add_event(Component.Event event) {
-        if (!days_events.add(event))
+        if (!days_events.add(event)) {
+            debug("Unable to add event %s to day pane for %s: already present", event.to_string(),
+                date.to_string());
+            
             return;
+        }
         
         event.notify[Component.Event.PROP_SUMMARY].connect(queue_draw);
         event.notify[Component.Event.PROP_DATE_SPAN].connect(on_update_date_time);
@@ -118,8 +122,12 @@ internal class DayPane : Pane, Common.InstanceContainer {
     }
     
     public void remove_event(Component.Event event) {
-        if (!days_events.remove(event))
+        if (!days_events.remove(event)) {
+            debug("Unable to remove event %s from day pane for %s: not present in sorted_events",
+                event.to_string(), date.to_string());
+            
             return;
+        }
         
         event.notify[Component.Event.PROP_SUMMARY].disconnect(queue_draw);
         event.notify[Component.Event.PROP_DATE_SPAN].disconnect(on_update_date_time);
@@ -137,11 +145,9 @@ internal class DayPane : Pane, Common.InstanceContainer {
     private void on_update_date_time(Object object, ParamSpec param) {
         Component.Event event = (Component.Event) object;
         
-        // remove entirely if not in this date any more, otherwise remove and re-add to re-sort
+        // remove entirely if not in this date any more
         if (!(date in event.get_event_date_span(Calendar.System.timezone)))
             remove_event(event);
-        else if (days_events.remove(event))
-            days_events.add(event);
         
         queue_draw();
     }
@@ -223,6 +229,20 @@ internal class DayPane : Pane, Common.InstanceContainer {
         return true;
     }
     
+    private bool filter_date_spanning_events(Component.Event event) {
+        // All-day events are handled in separate container ...
+        if (event.is_all_day)
+            return false;
+        
+        // ... as are events that span days (or outside this date, although that technically
+        // shouldn't happen)
+        Calendar.DateSpan date_span = event.get_event_date_span(Calendar.Timezone.local);
+        if (!date_span.is_same_day || !(date in date_span))
+            return false;
+        
+        return true;
+    }
+    
     // note that a painter's algorithm should be used here: background should be painted before
     // calling base method, and foreground afterward
     protected override bool on_draw(Cairo.Context ctx) {
@@ -237,17 +257,12 @@ internal class DayPane : Pane, Common.InstanceContainer {
         // each event is drawn with a slightly-transparent rectangle with a solid hairline bounding
         Palette.prepare_hairline(ctx, palette.border);
         
-        foreach (Component.Event event in days_events) {
-            // All-day events are handled in separate container ...
-            if (event.is_all_day)
-                continue;
-            
-            // ... as are events that span days (or outside this date, although that technically
-            // shouldn't happen)
-            Calendar.DateSpan date_span = event.get_event_date_span(Calendar.Timezone.local);
-            if (!date_span.is_same_day || !(date in date_span))
-                continue;
-            
+        // Can't persist events in TreeSet because mutation is not handled well, see
+        // https://bugzilla.gnome.org/show_bug.cgi?id=736444
+        Gee.TreeSet<Component.Event> sorted_events = traverse<Component.Event>(days_events)
+            .filter(filter_date_spanning_events)
+            .to_tree_set();
+        foreach (Component.Event event in sorted_events) {
             Calendar.WallTime start_time =
                 event.exact_time_span.start_exact_time.to_timezone(Calendar.Timezone.local).to_wall_time();
             Calendar.WallTime end_time =