Merge pull request #721 from nextcloud/fix/dispose_state

fix(neon): remove and dispose dagnling blocs after logout

packages/neon/neon/lib/src/bloc/bloc.dart

@@ -1,9 +1,11 @@
 import 'dart:async';
 
 import 'package:flutter/foundation.dart';
+import 'package:neon/src/models/disposable.dart';
 import 'package:neon/src/utils/request_manager.dart';
 
-abstract class Bloc {
+abstract class Bloc implements Disposable {
+  @override
   @mustCallSuper
   void dispose();
 }
@@ -44,17 +46,3 @@ abstract class InteractiveBloc extends Bloc {
     }
   }
 }
-
-extension DisposeableIterableBloc on Iterable<Bloc> {
-  void disposeAll() {
-    for (final bloc in this) {
-      bloc.dispose();
-    }
-  }
-}
-
-extension DisposeableMapBloc on Map<dynamic, Bloc> {
-  void disposeAll() {
-    values.disposeAll();
-  }
-}

packages/neon/neon/lib/src/blocs/accounts.dart

@@ -1,6 +1,7 @@
 import 'dart:async';
 import 'dart:convert';
 
+import 'package:collection/collection.dart';
 import 'package:flutter/foundation.dart';
 import 'package:meta/meta.dart';
 import 'package:neon/src/bloc/bloc.dart';
@@ -10,6 +11,7 @@ import 'package:neon/src/blocs/unified_search.dart';
 import 'package:neon/src/blocs/user_details.dart';
 import 'package:neon/src/blocs/user_statuses.dart';
 import 'package:neon/src/models/account.dart';
+import 'package:neon/src/models/account_cache.dart';
 import 'package:neon/src/models/app_implementation.dart';
 import 'package:neon/src/settings/models/storage.dart';
 import 'package:neon/src/utils/account_options.dart';
@@ -97,30 +99,40 @@ class AccountsBloc extends Bloc implements AccountsBlocEvents, AccountsBlocState
         setActiveAccount(as.first);
       }
     }
+
+    accounts.listen((final accounts) {
+      _accountsOptions.pruneAgainst(accounts);
+      _appsBlocs.pruneAgainst(accounts);
+      _capabilitiesBlocs.pruneAgainst(accounts);
+      _userDetailsBlocs.pruneAgainst(accounts);
+      _userStatusesBlocs.pruneAgainst(accounts);
+      _unifiedSearchBlocs.pruneAgainst(accounts);
+      for (final app in _allAppImplementations) {
+        app.blocsCache.pruneAgainst(accounts);
+      }
+    });
   }
 
   final GlobalOptions _globalOptions;
   final Iterable<AppImplementation> _allAppImplementations;
 
-  final _accountsOptions = <String, AccountSpecificOptions>{};
+  final _accountsOptions = AccountCache<AccountSpecificOptions>();
-  final _appsBlocs = <String, AppsBloc>{};
+  final _appsBlocs = AccountCache<AppsBloc>();
-  final _capabilitiesBlocs = <String, CapabilitiesBloc>{};
+  final _capabilitiesBlocs = AccountCache<CapabilitiesBloc>();
-  final _userDetailsBlocs = <String, UserDetailsBloc>{};
+  final _userDetailsBlocs = AccountCache<UserDetailsBloc>();
-  final _userStatusesBlocs = <String, UserStatusesBloc>{};
+  final _userStatusesBlocs = AccountCache<UserStatusesBloc>();
-  final _unifiedSearchBlocs = <String, UnifiedSearchBloc>{};
+  final _unifiedSearchBlocs = AccountCache<UnifiedSearchBloc>();
 
   @override
   void dispose() {
     unawaited(activeAccount.close());
     unawaited(accounts.close());
-    _appsBlocs.disposeAll();
+    _appsBlocs.dispose();
-    _capabilitiesBlocs.disposeAll();
+    _capabilitiesBlocs.dispose();
-    _userDetailsBlocs.disposeAll();
+    _userDetailsBlocs.dispose();
-    _userStatusesBlocs.disposeAll();
+    _userStatusesBlocs.dispose();
-    _unifiedSearchBlocs.disposeAll();
+    _unifiedSearchBlocs.dispose();
-    for (final options in _accountsOptions.values) {
+    _accountsOptions.dispose();
-      options.dispose();
-    }
   }
 
   @override
@@ -212,8 +224,7 @@ class AccountsBloc extends Bloc implements AccountsBlocEvents, AccountsBlocState
   /// The options for the specified [account].
   ///
   /// Use [activeOptions] to get them for the [activeAccount].
-  AccountSpecificOptions getOptionsFor(final Account account) =>
+  AccountSpecificOptions getOptionsFor(final Account account) => _accountsOptions[account] ??= AccountSpecificOptions(
-      _accountsOptions[account.id] ??= AccountSpecificOptions(
         AppStorage(StorageKeys.accounts, account.id),
         getAppsBlocFor(account),
       );
@@ -226,7 +237,7 @@ class AccountsBloc extends Bloc implements AccountsBlocEvents, AccountsBlocState
   /// The appsBloc for the specified [account].
   ///
   /// Use [activeAppsBloc] to get them for the [activeAccount].
-  AppsBloc getAppsBlocFor(final Account account) => _appsBlocs[account.id] ??= AppsBloc(
+  AppsBloc getAppsBlocFor(final Account account) => _appsBlocs[account] ??= AppsBloc(
         getCapabilitiesBlocFor(account),
         this,
         account,
@@ -242,7 +253,7 @@ class AccountsBloc extends Bloc implements AccountsBlocEvents, AccountsBlocState
   ///
   /// Use [activeCapabilitiesBloc] to get them for the [activeAccount].
   CapabilitiesBloc getCapabilitiesBlocFor(final Account account) =>
-      _capabilitiesBlocs[account.id] ??= CapabilitiesBloc(account);
+      _capabilitiesBlocs[account] ??= CapabilitiesBloc(account);
 
   /// The userDetailsBloc for the [activeAccount].
   ///
@@ -253,7 +264,7 @@ class AccountsBloc extends Bloc implements AccountsBlocEvents, AccountsBlocState
   ///
   /// Use [activeUerDetailsBloc] to get them for the [activeAccount].
   UserDetailsBloc getUserDetailsBlocFor(final Account account) =>
-      _userDetailsBlocs[account.id] ??= UserDetailsBloc(account);
+      _userDetailsBlocs[account] ??= UserDetailsBloc(account);
 
   /// The userStatusBloc for the [activeAccount].
   ///
@@ -264,7 +275,7 @@ class AccountsBloc extends Bloc implements AccountsBlocEvents, AccountsBlocState
   ///
   /// Use [activeUserStatusesBloc] to get them for the [activeAccount].
   UserStatusesBloc getUserStatusesBlocFor(final Account account) =>
-      _userStatusesBlocs[account.id] ??= UserStatusesBloc(account);
+      _userStatusesBlocs[account] ??= UserStatusesBloc(account);
 
   /// The UnifiedSearchBloc for the [activeAccount].
   ///
@@ -275,7 +286,7 @@ class AccountsBloc extends Bloc implements AccountsBlocEvents, AccountsBlocState
   ///
   /// Use [activeUnifiedSearchBloc] to get them for the [activeAccount].
   UnifiedSearchBloc getUnifiedSearchBlocFor(final Account account) =>
-      _unifiedSearchBlocs[account.id] ??= UnifiedSearchBloc(
+      _unifiedSearchBlocs[account] ??= UnifiedSearchBloc(
         getAppsBlocFor(account),
         account,
       );

packages/neon/neon/lib/src/blocs/apps.dart

@@ -58,6 +58,13 @@ class AppsBloc extends InteractiveBloc implements AppsBlocEvents, AppsBlocStates
         return;
       }
 
+      // dispose unsupported apps
+      for (final app in _allAppImplementations) {
+        if (result.requireData.tryFind(app.id) == null) {
+          app.blocsCache.remove(_account);
+        }
+      }
+
       final options = _accountsBloc.getOptionsFor(_account);
       final initialApp = options.initialApp.value ?? _getInitialAppFallback();
       if (initialApp != null) {

packages/neon/neon/lib/src/models/account_cache.dart

@@ -0,0 +1,49 @@
+import 'package:neon/models.dart';
+import 'package:neon/src/models/disposable.dart';
+
+/// Cache for [Account] specific [Disposable] objects.
+class AccountCache<T extends Disposable> implements Disposable {
+  AccountCache();
+
+  final Map<String, T> _cache = {};
+
+  /// Disposes all entries and clears the cache.
+  @override
+  void dispose() {
+    _cache
+      ..disposeAll()
+      ..clear();
+  }
+
+  /// Updates the cache against the given [accounts].
+  ///
+  /// Every cache entry with an account no longer in [accounts] is removed and
+  /// disposed. This method is in O(N²).
+  void pruneAgainst(final Iterable<Account> accounts) {
+    _cache.removeWhere((final key, final value) {
+      if (accounts.tryFind(key) == null) {
+        value.dispose();
+        return true;
+      }
+
+      return false;
+    });
+  }
+
+  /// Removes [account] and its associated value, if present, from the cache.
+  ///
+  /// If present the value associated with `account` is disposed.
+  void remove(final Account? account) {
+    final removed = _cache.remove(account?.id);
+    removed?.dispose();
+  }
+
+  /// The value for the given [account], or `null` if [account] is not in the cache.
+  T? operator [](final Account account) => _cache[account.id];
+
+  /// Associates the [account] with the given [value].
+  ///
+  /// If the account was already in the cache, its associated value is changed.
+  /// Otherwise the account/value pair is added to the cache.
+  void operator []=(final Account account, final T value) => _cache[account.id] = value;
+}

packages/neon/neon/lib/src/models/app_implementation.dart

@@ -6,6 +6,8 @@ import 'package:neon/l10n/localizations.dart';
 import 'package:neon/src/bloc/bloc.dart';
 import 'package:neon/src/blocs/accounts.dart';
 import 'package:neon/src/models/account.dart';
+import 'package:neon/src/models/account_cache.dart';
+import 'package:neon/src/models/disposable.dart';
 import 'package:neon/src/settings/models/options_collection.dart';
 import 'package:neon/src/settings/models/storage.dart';
 import 'package:neon/src/widgets/drawer_destination.dart';
@@ -14,7 +16,7 @@ import 'package:rxdart/rxdart.dart';
 import 'package:vector_graphics/vector_graphics.dart';
 
 @immutable
-abstract class AppImplementation<T extends Bloc, R extends NextcloudAppOptions> {
+abstract class AppImplementation<T extends Bloc, R extends NextcloudAppOptions> implements Disposable {
   AppImplementation();
 
   String get id;
@@ -30,9 +32,9 @@ abstract class AppImplementation<T extends Bloc, R extends NextcloudAppOptions>
   @mustBeOverridden
   R get options;
 
-  final Map<String, T> _blocs = {};
+  final blocsCache = AccountCache<T>();
 
-  T getBloc(final Account account) => _blocs[account.id] ??= buildBloc(account);
+  T getBloc(final Account account) => blocsCache[account] ??= buildBloc(account);
 
   @protected
   T buildBloc(final Account account);
@@ -110,10 +112,11 @@ abstract class AppImplementation<T extends Bloc, R extends NextcloudAppOptions>
         },
       );
 
+  @override
   @mustCallSuper
   void dispose() {
     options.dispose();
-    _blocs.disposeAll();
+    blocsCache.dispose();
   }
 
   /// A custom theme that will be injected into the widget tree.

packages/neon/neon/lib/src/models/disposable.dart

@@ -0,0 +1,33 @@
+import 'package:meta/meta.dart';
+
+/// Interface of a disposable class.
+abstract interface class Disposable {
+  /// Discards any resources used by the object. After this is called, the
+  /// object is not in a usable state and should be discarded (calls to
+  /// streams or other state should throw after the object is disposed).
+  ///
+  /// This method should only be called by the object's owner.
+  @mustCallSuper
+  void dispose();
+}
+
+extension DisposeableIterableBloc on Iterable<Disposable> {
+  /// Calls [Disposable.dispose] on all entries.
+  ///
+  /// The disposed values will not be removed from the iteraable.
+  void disposeAll() {
+    for (final bloc in this) {
+      bloc.dispose();
+    }
+  }
+}
+
+extension DisposeableMapBloc on Map<dynamic, Disposable> {
+  /// Calls [Disposable.dispose] on all entries.
+  ///
+  /// The disposed values will not be removed from the map.
+  /// Call [clear] to remove them.
+  void disposeAll() {
+    values.disposeAll();
+  }
+}

packages/neon/neon/lib/src/settings/models/option.dart

@@ -3,6 +3,7 @@ import 'dart:async';
 import 'package:collection/collection.dart';
 import 'package:flutter/foundation.dart';
 import 'package:meta/meta.dart';
+import 'package:neon/src/models/disposable.dart';
 import 'package:neon/src/models/label_builder.dart';
 import 'package:neon/src/settings/models/options_category.dart';
 import 'package:neon/src/settings/models/storage.dart';
@@ -13,7 +14,7 @@ import 'package:rxdart/rxdart.dart';
 /// See:
 ///   * [ToggleOption] for an Option<bool>
 ///   * [SelectOption] for an Option with multiple values
-sealed class Option<T> extends ChangeNotifier implements ValueListenable<T> {
+sealed class Option<T> extends ChangeNotifier implements ValueListenable<T>, Disposable {
   /// Creates an Option
   Option({
     required this.storage,

packages/neon/neon/lib/src/settings/models/options_collection.dart

@@ -1,11 +1,12 @@
 import 'package:meta/meta.dart';
+import 'package:neon/src/models/disposable.dart';
 import 'package:neon/src/settings/models/exportable.dart';
 import 'package:neon/src/settings/models/option.dart';
 import 'package:neon/src/settings/models/options_category.dart';
 import 'package:neon/src/settings/models/storage.dart';
 
 /// Collection of [Option]s.
-abstract class OptionsCollection implements Exportable {
+abstract class OptionsCollection implements Exportable, Disposable {
   OptionsCollection(this.storage);
 
   /// Storage backend to use.
@@ -30,10 +31,9 @@ abstract class OptionsCollection implements Exportable {
   ///
   /// Implementers extending this must call super.
   @mustCallSuper
+  @override
   void dispose() {
-    for (final option in options) {
+    options.disposeAll();
-      option.dispose();
-    }
   }
 
   @override

packages/neon/neon/test/account_cache_test.dart

@@ -0,0 +1,74 @@
+import 'package:mocktail/mocktail.dart';
+import 'package:neon/models.dart';
+import 'package:neon/src/models/account_cache.dart';
+import 'package:neon/src/models/disposable.dart';
+import 'package:test/test.dart';
+
+class DisposableMock extends Mock implements Disposable {}
+
+// ignore: avoid_implementing_value_types
+class AccountMock extends Mock implements Account {}
+
+void main() {
+  final disposable0 = DisposableMock();
+  final disposable1 = DisposableMock();
+  final account0 = AccountMock();
+  final account1 = AccountMock();
+
+  when(() => account0.id).thenReturn('key0');
+  when(() => account1.id).thenReturn('key1');
+
+  group('AccountCache', () {
+    test('map functionality', () {
+      final cache = AccountCache<DisposableMock>();
+
+      expect(cache[account0], isNull);
+
+      cache[account0] = disposable0;
+      expect(cache[account0], disposable0);
+
+      expect(cache[account0] ??= disposable1, disposable0);
+      expect(cache[account1] ??= disposable1, disposable1);
+    });
+
+    test('prune', () {
+      final cache = AccountCache<DisposableMock>();
+      cache[account0] = disposable0;
+      cache[account1] = disposable1;
+
+      cache.pruneAgainst([account0]);
+
+      expect(cache[account0], disposable0);
+      expect(cache[account1], isNull);
+      verify(disposable1.dispose).called(1);
+    });
+
+    test('dispose', () {
+      final cache = AccountCache<DisposableMock>();
+      cache[account0] = disposable0;
+      cache[account1] = disposable1;
+
+      cache.dispose();
+
+      expect(cache[account0], isNull);
+      expect(cache[account1], isNull);
+      verify(disposable0.dispose).called(1);
+      verify(disposable1.dispose).called(1);
+    });
+
+    test('remove', () {
+      final cache = AccountCache<DisposableMock>();
+      cache[account0] = disposable0;
+      cache[account1] = disposable1;
+
+      cache.remove(null);
+
+      expect(cache[account0], disposable0);
+      expect(cache[account1], disposable1);
+
+      cache.remove(account0);
+      expect(cache[account0], isNull);
+      verify(disposable0.dispose).called(1);
+    });
+  });
+}

packages/neon/neon/test/disposable_test.dart

@@ -0,0 +1,39 @@
+// ignore_for_file: cascade_invocations
+
+import 'package:mocktail/mocktail.dart';
+import 'package:neon/src/models/disposable.dart';
+import 'package:test/test.dart';
+
+class DisposableMock extends Mock implements Disposable {}
+
+void main() {
+  test('Disposable extensions', () {
+    final disposable0 = DisposableMock();
+    final disposable1 = DisposableMock();
+    final disposable3 = DisposableMock();
+
+    final list = [
+      disposable0,
+      disposable1,
+      disposable3,
+    ];
+
+    list.disposeAll();
+
+    verify(disposable0.dispose).called(1);
+    verify(disposable1.dispose).called(1);
+    verify(disposable3.dispose).called(1);
+
+    final map = {
+      'disposable0': disposable0,
+      'disposable1': disposable1,
+      'disposable3': disposable3,
+    };
+
+    map.disposeAll();
+
+    verify(disposable0.dispose).called(1);
+    verify(disposable1.dispose).called(1);
+    verify(disposable3.dispose).called(1);
+  });
+}