refactor: tray and window behaviours #1298
Conversation
Using a bunch of state listeners to observe when tray should reload, rather than each piece of state telling the tray when to update (which is an anti-pattern, confusing, and bug prone).
| @@ -79,7 +79,7 @@ class App extends StatelessWidget { | |||
|
|
|||
| /// A cubit which manages the system tray entries | |||
| BlocProvider<TrayCubit>( | |||
| create: (_) => TrayCubit()..initialize(), | |||
| create: (_) => TrayCubit(), | |||
There was a problem hiding this comment.
This gets called in a more appropriate place now.
| @@ -32,7 +31,6 @@ class FavoriteBloc extends LoggingBloc<FavoriteEvent, FavoritesState> { | |||
| return; | |||
| } | |||
| emit(FavoritesLoaded(favs.values)); | |||
| App.navState.currentContext?.read<TrayCubit>().reloadFavorites(); | |||
There was a problem hiding this comment.
Decoupling - remove all "pushed" updates to the tray.
| @@ -38,12 +37,10 @@ class ProfileBloc extends LoggingBloc<ProfileEvent, ProfileState> { | |||
|
|
|||
| if (profile == null) { | |||
| emit(ProfileFailedLoad(uuid)); | |||
| App.navState.currentContext?.read<TrayCubit>().reloadFavorites(); | |||
| @@ -87,34 +87,50 @@ class TrayCubit extends LoggingCubit<TrayState> { | |||
| emit(const TrayLoaded()); | |||
| } | |||
|
|
|||
| Future<void> reloadFavorites() async { | |||
| Future<void> reload() async { | |||
There was a problem hiding this comment.
No functional changes to the reload tray method, but I did document it for clarity
| /// PERF: We should conditionally call setContextMenu if there was a state | ||
| /// change which resulted in an actual change to the favorites list. | ||
| /// Currently we just force call updates which is really inefficient |
There was a problem hiding this comment.
The lists that we iterate through are not expected to be long, but in the future, if someone were to have 1000s (probably an even bigger magnitude) of profiles loaded in the app we will have to determine if the tray reloading needs to be optimized - reducing useless reloads may help
| /// Must strongly type [context] here or Dart will infer the wrong type for | ||
| /// the [.read()] extension which causes an error | ||
| void reloadTray(BuildContext context, _) { | ||
| context.read<TrayCubit>().reload(); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is the listener which triggers tray reloads whenever a piece of state changes.
| listeners: [ | ||
| /// Reload the tray whenever one of the following states changes | ||
| /// Note: this doesn't always result in a change to the tray, but we | ||
| /// still have to check | ||
| BlocListener<FavoriteBloc, FavoritesState>( | ||
| listener: reloadTray, | ||
| ), | ||
| BlocListener<ProfileListBloc, ProfileListState>( | ||
| listener: reloadTray, | ||
| ), | ||
| BlocListener<ProfilesRunningCubit, ProfilesRunningState>( | ||
| listener: reloadTray, | ||
| ), | ||
|
|
||
| /// Yeah I really hate this... an indefinite list of listeners | ||
| /// but it's the only way to decouple the profiles from having to know | ||
| /// about the tray | ||
| /// | ||
| /// The tray should know about profiles, profiles should not know | ||
| /// about tray. Even if it is slightly more costly in performance, the | ||
| /// calls where we take the performance hit are: | ||
| /// 1. In an asynchronous background task (who cares) | ||
| /// 2. Worth it, compared to the potential maintenance costs | ||
| ...profiles.map((uuid) => BlocProvider<ProfileBloc>( | ||
| key: Key("TrayManager-$uuid"), | ||
| create: (context) => profileCacheCubit.getProfileBloc(uuid), | ||
| child: BlocListener<ProfileBloc, ProfileState>( | ||
| listener: reloadTray, | ||
| ), | ||
| )), | ||
| ], |
There was a problem hiding this comment.
All of these are the new listeners which will trigger the tray to reload whenever any of these change - could be an edited profile, favorited / unfavorited, start / stop, etc.
There was a problem hiding this comment.
I dislike that we need to iterate through each profile and add it as a listener, we could optimize by filtering to only favorited profiles, but that introduces edge cases not worth dealing with right now.
| @@ -6,5 +8,11 @@ import 'app.dart'; | |||
| Future<void> main() async { | |||
| WidgetsFlutterBinding.ensureInitialized(); | |||
| windowManager.ensureInitialized(); | |||
| runApp(const App()); | |||
| try { | |||
| await windowManager.setSkipTaskbar(true); // Don't show the app icon in dock | |||
There was a problem hiding this comment.
Disable dock icon - the dock icon wasn't functional, and made it look like the app is hung, but really the app is only functional from the tray.
- What I did
Decoupled the tray feature from the rest of the app - tray should know about other features, but other features should not have to know about the tray. We want the tray to pull updates from the app state, the app state shouldn't have to push updates to the tray.
Removed the app icon from the dock, it's just kind of there and doesn't work to create a new window when you click it. The app should be interacted with from the tray, not the dock.
- How I did it
- How to verify it
- Description for the changelog
refactor: tray and window behaviours