SortedNotesFolder: Avoid calling sort again and again

For long lists of notes this could get very very expensive.

lib/core/note.dart

@@ -1,6 +1,6 @@
 import 'dart:io';
 
-import 'package:flutter/material.dart';
+import 'package:gitjournal/core/note_notifier.dart';
 import 'package:gitjournal/settings.dart';
 import 'package:gitjournal/utils/markdown.dart';
 import 'package:path/path.dart' as p;
@@ -18,7 +18,7 @@ enum NoteLoadState {
   NotExists,
 }
 
-class Note with ChangeNotifier {
+class Note with NotesNotifier {
   NotesFolder parent;
   String _filePath;
 
@@ -60,7 +60,7 @@ class Note with ChangeNotifier {
     if (!canHaveMetadata) return;
 
     _created = dt;
-    notifyListeners();
+    _notifyModified();
   }
 
   DateTime get modified {
@@ -71,12 +71,12 @@ class Note with ChangeNotifier {
     if (!canHaveMetadata) return;
 
     _modified = dt;
-    notifyListeners();
+    _notifyModified();
   }
 
   void updateModified() {
     modified = DateTime.now();
-    notifyListeners();
+    _notifyModified();
   }
 
   String get body {
@@ -86,7 +86,7 @@ class Note with ChangeNotifier {
   set body(String newBody) {
     _body = newBody;
     _summary = null;
-    notifyListeners();
+    _notifyModified();
   }
 
   String get title {
@@ -97,7 +97,7 @@ class Note with ChangeNotifier {
     if (!canHaveMetadata) return;
 
     _title = title;
-    notifyListeners();
+    _notifyModified();
   }
 
   bool get canHaveMetadata {
@@ -113,7 +113,7 @@ class Note with ChangeNotifier {
     _data = data;
     noteSerializer.decode(_data, this);
 
-    notifyListeners();
+    _notifyModified();
   }
 
   bool isEmpty() {
@@ -149,7 +149,7 @@ class Note with ChangeNotifier {
 
     if (!file.existsSync()) {
       _loadState = NoteLoadState.NotExists;
-      notifyListeners();
+      _notifyModified();
       return _loadState;
     }
 
@@ -160,7 +160,7 @@ class Note with ChangeNotifier {
     fileLastModified = file.lastModifiedSync();
     _loadState = NoteLoadState.Loaded;
 
-    notifyListeners();
+    _notifyModified();
     return _loadState;
   }
 
@@ -198,7 +198,7 @@ class Note with ChangeNotifier {
     }
     _filePath = newFilePath;
 
-    notifyListeners();
+    _notifyModified();
   }
 
   bool move(NotesFolder destFolder) {
@@ -213,7 +213,7 @@ class Note with ChangeNotifier {
     parent = destFolder;
     destFolder.add(this);
 
-    notifyListeners();
+    _notifyModified();
     return true;
   }
 
@@ -232,4 +232,9 @@ class Note with ChangeNotifier {
   String toString() {
     return 'Note{filePath: $_filePath, created: $created, modified: $modified, data: $_data}';
   }
+
+  void _notifyModified() {
+    notifyModifiedListeners(this);
+    notifyListeners();
+  }
 }

lib/core/note_notifier.dart

@@ -0,0 +1,178 @@
+import 'package:flutter/material.dart';
+import 'package:flutter/foundation.dart';
+
+import 'note.dart';
+
+typedef NoteModificationCallback = void Function(Note note);
+
+class NotesNotifier implements ChangeNotifier {
+  var _modListeners = ObserverList<NoteModificationCallback>();
+
+  void addModifiedListener(NoteModificationCallback listener) {
+    _modListeners.add(listener);
+  }
+
+  void removeModifiedListener(NoteModificationCallback listener) {
+    _modListeners.remove(listener);
+  }
+
+  @mustCallSuper
+  @override
+  void dispose() {
+    assert(_debugAssertNotDisposed());
+    _modListeners = null;
+    _listeners = null;
+  }
+
+  //
+  // ChangeNotifier implementation - How to not duplicate this?
+  //
+  ObserverList<VoidCallback> _listeners = ObserverList<VoidCallback>();
+
+  bool _debugAssertNotDisposed() {
+    assert(() {
+      if (_listeners == null) {
+        throw FlutterError.fromParts(<DiagnosticsNode>[
+          ErrorSummary('A $runtimeType was used after being disposed.'),
+          ErrorDescription(
+              'Once you have called dispose() on a $runtimeType, it can no longer be used.')
+        ]);
+      }
+      return true;
+    }());
+    return true;
+  }
+
+  /// Whether any listeners are currently registered.
+  ///
+  /// Clients should not depend on this value for their behavior, because having
+  /// one listener's logic change when another listener happens to start or stop
+  /// listening will lead to extremely hard-to-track bugs. Subclasses might use
+  /// this information to determine whether to do any work when there are no
+  /// listeners, however; for example, resuming a [Stream] when a listener is
+  /// added and pausing it when a listener is removed.
+  ///
+  /// Typically this is used by overriding [addListener], checking if
+  /// [hasListeners] is false before calling `super.addListener()`, and if so,
+  /// starting whatever work is needed to determine when to call
+  /// [notifyListeners]; and similarly, by overriding [removeListener], checking
+  /// if [hasListeners] is false after calling `super.removeListener()`, and if
+  /// so, stopping that same work.
+  @protected
+  @override
+  bool get hasListeners {
+    assert(_debugAssertNotDisposed());
+    return _listeners.isNotEmpty;
+  }
+
+  /// Register a closure to be called when the object changes.
+  ///
+  /// This method must not be called after [dispose] has been called.
+  @override
+  void addListener(VoidCallback listener) {
+    assert(_debugAssertNotDisposed());
+    _listeners.add(listener);
+  }
+
+  /// Remove a previously registered closure from the list of closures that are
+  /// notified when the object changes.
+  ///
+  /// If the given listener is not registered, the call is ignored.
+  ///
+  /// This method must not be called after [dispose] has been called.
+  ///
+  /// If a listener had been added twice, and is removed once during an
+  /// iteration (i.e. in response to a notification), it will still be called
+  /// again. If, on the other hand, it is removed as many times as it was
+  /// registered, then it will no longer be called. This odd behavior is the
+  /// result of the [ChangeNotifier] not being able to determine which listener
+  /// is being removed, since they are identical, and therefore conservatively
+  /// still calling all the listeners when it knows that any are still
+  /// registered.
+  ///
+  /// This surprising behavior can be unexpectedly observed when registering a
+  /// listener on two separate objects which are both forwarding all
+  /// registrations to a common upstream object.
+  @override
+  void removeListener(VoidCallback listener) {
+    assert(_debugAssertNotDisposed());
+    _listeners.remove(listener);
+  }
+
+  /// Call all the registered listeners.
+  ///
+  /// Call this method whenever the object changes, to notify any clients the
+  /// object may have. Listeners that are added during this iteration will not
+  /// be visited. Listeners that are removed during this iteration will not be
+  /// visited after they are removed.
+  ///
+  /// Exceptions thrown by listeners will be caught and reported using
+  /// [FlutterError.reportError].
+  ///
+  /// This method must not be called after [dispose] has been called.
+  ///
+  /// Surprising behavior can result when reentrantly removing a listener (i.e.
+  /// in response to a notification) that has been registered multiple times.
+  /// See the discussion at [removeListener].
+  @protected
+  @visibleForTesting
+  @override
+  void notifyListeners() {
+    assert(_debugAssertNotDisposed());
+    if (_listeners != null) {
+      final List<VoidCallback> localListeners =
+          List<VoidCallback>.from(_listeners);
+      for (VoidCallback listener in localListeners) {
+        try {
+          if (_listeners.contains(listener)) {
+            listener();
+          }
+        } catch (exception, stack) {
+          FlutterError.reportError(FlutterErrorDetails(
+            exception: exception,
+            stack: stack,
+            library: 'foundation library',
+            context: ErrorDescription(
+                'while dispatching notifications for $runtimeType'),
+            informationCollector: () sync* {
+              yield DiagnosticsProperty<ChangeNotifier>(
+                'The $runtimeType sending notification was',
+                this,
+                style: DiagnosticsTreeStyle.errorProperty,
+              );
+            },
+          ));
+        }
+      }
+    }
+  }
+
+  void notifyModifiedListeners(Note note) {
+    assert(_debugAssertNotDisposed());
+    if (_modListeners != null) {
+      final localListeners = List<NoteModificationCallback>.from(_modListeners);
+      for (NoteModificationCallback listener in localListeners) {
+        try {
+          if (_modListeners.contains(listener)) {
+            listener(note);
+          }
+        } catch (exception, stack) {
+          FlutterError.reportError(FlutterErrorDetails(
+            exception: exception,
+            stack: stack,
+            library: 'foundation library',
+            context: ErrorDescription(
+                'while dispatching notifications for $runtimeType'),
+            informationCollector: () sync* {
+              yield DiagnosticsProperty<ChangeNotifier>(
+                'The $runtimeType sending notification was',
+                this,
+                style: DiagnosticsTreeStyle.errorProperty,
+              );
+            },
+          ));
+        }
+      }
+    }
+  }
+}

lib/core/notes_folder.dart

@@ -38,6 +38,10 @@ class NotesFolder
     notifyListeners();
   }
 
+  void _noteModified(Note note) {
+    notifyNoteModified(-1, note);
+  }
+
   void reset(String folderPath) {
     _folderPath = folderPath;
 
@@ -183,7 +187,7 @@ class NotesFolder
         continue;
       }
       Fimber.d("Found file ${fsEntity.path}");
-      note.addListener(_entityChanged);
+      note.addModifiedListener(_noteModified);
 
       _notes.add(note);
       _entityMap[fsEntity.path] = note;
@@ -198,11 +202,11 @@ class NotesFolder
       assert(e != null);
 
       assert(e is NotesFolder || e is Note);
-      e.removeListener(_entityChanged);
       _entityMap.remove(path);
 
       if (e is Note) {
         Fimber.d("File $path was no longer found");
+        e.removeModifiedListener(_noteModified);
         var i = _notes.indexWhere((n) => n.filePath == path);
         assert(i != -1);
         var note = _notes[i];
@@ -210,6 +214,7 @@ class NotesFolder
         notifyNoteRemoved(i, note);
       } else {
         Fimber.d("Folder $path was no longer found");
+        e.removeListener(_entityChanged);
         var i = _folders.indexWhere((f) => f.folderPath == path);
         assert(i != -1);
         var folder = _folders[i];

lib/core/notes_folder_notifier.dart

@@ -14,6 +14,7 @@ class NotesFolderNotifier implements ChangeNotifier {
 
   var _noteAddedListeners = ObserverList<NoteNotificationCallback>();
   var _noteRemovedListeners = ObserverList<NoteNotificationCallback>();
+  var _noteModifiedListeners = ObserverList<NoteNotificationCallback>();
 
   void addFolderRemovedListener(FolderNotificationCallback listener) {
     _folderRemovedListeners.add(listener);
@@ -47,6 +48,14 @@ class NotesFolderNotifier implements ChangeNotifier {
     _noteRemovedListeners.remove(listener);
   }
 
+  void addNoteModifiedListener(NoteNotificationCallback listener) {
+    _noteModifiedListeners.add(listener);
+  }
+
+  void removeNoteModifiedListener(NoteNotificationCallback listener) {
+    _noteModifiedListeners.remove(listener);
+  }
+
   @mustCallSuper
   @override
   void dispose() {
@@ -54,6 +63,7 @@ class NotesFolderNotifier implements ChangeNotifier {
     _folderRemovedListeners = null;
     _noteAddedListeners = null;
     _noteRemovedListeners = null;
+    _noteModifiedListeners = null;
 
     assert(_debugAssertNotDisposed());
     _listeners = null;
@@ -143,6 +153,10 @@ class NotesFolderNotifier implements ChangeNotifier {
     _notifyNoteCallback(_noteRemovedListeners, index, note);
   }
 
+  void notifyNoteModified(int index, Note note) {
+    _notifyNoteCallback(_noteModifiedListeners, index, note);
+  }
+
   //
   // ChangeNotifier implementation - How to not duplicate this?
   //

lib/core/sorted_notes_folder.dart

@@ -30,8 +30,7 @@ class SortedNotesFolder
 
     folder.addNoteAddedListener(_noteAddedListener);
     folder.addNoteRemovedListener(_noteRemovedListener);
-
+    folder.addNoteModifiedListener(_noteModifiedListener);
-    folder.addListener(_entityChanged);
   }
 
   @override
@@ -41,8 +40,7 @@ class SortedNotesFolder
 
     folder.removeNoteAddedListener(_noteAddedListener);
     folder.removeNoteRemovedListener(_noteRemovedListener);
-
+    folder.removeNoteModifiedListener(_noteModifiedListener);
-    folder.removeListener(_entityChanged);
 
     super.dispose();
   }
@@ -63,14 +61,7 @@ class SortedNotesFolder
       return;
     }
 
-    var i = 0;
+    var i = _insertInCorrectPos(note);
-    for (; i < _notes.length; i++) {
-      var n = _notes[i];
-      if (_sortFunc(n, note) > 0) {
-        break;
-      }
-    }
-    _notes.insert(i, note);
     notifyNoteAdded(i, note);
   }
 
@@ -84,11 +75,28 @@ class SortedNotesFolder
     notifyNoteRemoved(index, note);
   }
 
-  void _entityChanged() {
+  void _noteModifiedListener(int _, Note note) {
-    _notes.sort(_sortFunc);
+    var i = _notes.indexWhere((Note n) => note.filePath == n.filePath);
+    assert(i != -1);
+
+    _notes.removeAt(i);
+    _insertInCorrectPos(note);
+
     notifyListeners();
   }
 
+  int _insertInCorrectPos(Note note) {
+    var i = 0;
+    for (; i < _notes.length; i++) {
+      var n = _notes[i];
+      if (_sortFunc(n, note) > 0) {
+        break;
+      }
+    }
+    _notes.insert(i, note);
+    return i;
+  }
+
   @override
   List<Note> get notes => _notes;
 

test/sorting_mode_test.dart

@@ -1,5 +1,3 @@
-import 'dart:io';
-
 import 'package:gitjournal/core/note.dart';
 import 'package:gitjournal/core/notes_folder.dart';
 import 'package:gitjournal/core/sorting_mode.dart';