From f0eeefe1251bc85059df2c7f3a65c5bb9de09e41 Mon Sep 17 00:00:00 2001 From: Vishesh Handa Date: Sun, 8 Mar 2020 01:15:48 +0100 Subject: [PATCH] SortedNotesFolder: Avoid calling sort again and again For long lists of notes this could get very very expensive. --- lib/core/note.dart | 29 +++-- lib/core/note_notifier.dart | 178 ++++++++++++++++++++++++++++ lib/core/notes_folder.dart | 9 +- lib/core/notes_folder_notifier.dart | 14 +++ lib/core/sorted_notes_folder.dart | 36 +++--- test/sorting_mode_test.dart | 2 - 6 files changed, 238 insertions(+), 30 deletions(-) create mode 100644 lib/core/note_notifier.dart diff --git a/lib/core/note.dart b/lib/core/note.dart index b60db201..b5081ec3 100644 --- a/lib/core/note.dart +++ b/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(); + } } diff --git a/lib/core/note_notifier.dart b/lib/core/note_notifier.dart new file mode 100644 index 00000000..499d482d --- /dev/null +++ b/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(); + + 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 _listeners = ObserverList(); + + bool _debugAssertNotDisposed() { + assert(() { + if (_listeners == null) { + throw FlutterError.fromParts([ + 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 localListeners = + List.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( + 'The $runtimeType sending notification was', + this, + style: DiagnosticsTreeStyle.errorProperty, + ); + }, + )); + } + } + } + } + + void notifyModifiedListeners(Note note) { + assert(_debugAssertNotDisposed()); + if (_modListeners != null) { + final localListeners = List.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( + 'The $runtimeType sending notification was', + this, + style: DiagnosticsTreeStyle.errorProperty, + ); + }, + )); + } + } + } + } +} diff --git a/lib/core/notes_folder.dart b/lib/core/notes_folder.dart index 2971e41e..eeb04e2d 100644 --- a/lib/core/notes_folder.dart +++ b/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]; diff --git a/lib/core/notes_folder_notifier.dart b/lib/core/notes_folder_notifier.dart index 936a0f18..5ca6e5aa 100644 --- a/lib/core/notes_folder_notifier.dart +++ b/lib/core/notes_folder_notifier.dart @@ -14,6 +14,7 @@ class NotesFolderNotifier implements ChangeNotifier { var _noteAddedListeners = ObserverList(); var _noteRemovedListeners = ObserverList(); + var _noteModifiedListeners = ObserverList(); 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? // diff --git a/lib/core/sorted_notes_folder.dart b/lib/core/sorted_notes_folder.dart index 108e3621..5e1326a3 100644 --- a/lib/core/sorted_notes_folder.dart +++ b/lib/core/sorted_notes_folder.dart @@ -30,8 +30,7 @@ class SortedNotesFolder folder.addNoteAddedListener(_noteAddedListener); folder.addNoteRemovedListener(_noteRemovedListener); - - folder.addListener(_entityChanged); + folder.addNoteModifiedListener(_noteModifiedListener); } @override @@ -41,8 +40,7 @@ class SortedNotesFolder folder.removeNoteAddedListener(_noteAddedListener); folder.removeNoteRemovedListener(_noteRemovedListener); - - folder.removeListener(_entityChanged); + folder.removeNoteModifiedListener(_noteModifiedListener); super.dispose(); } @@ -63,14 +61,7 @@ class SortedNotesFolder return; } - var i = 0; - for (; i < _notes.length; i++) { - var n = _notes[i]; - if (_sortFunc(n, note) > 0) { - break; - } - } - _notes.insert(i, note); + var i = _insertInCorrectPos(note); notifyNoteAdded(i, note); } @@ -84,11 +75,28 @@ class SortedNotesFolder notifyNoteRemoved(index, note); } - void _entityChanged() { - _notes.sort(_sortFunc); + void _noteModifiedListener(int _, Note note) { + 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 get notes => _notes; diff --git a/test/sorting_mode_test.dart b/test/sorting_mode_test.dart index 8b5ed45d..a6d418b4 100644 --- a/test/sorting_mode_test.dart +++ b/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';