From 1bcf9fc73fb5d94d093cac87e850aa7afee880e3 Mon Sep 17 00:00:00 2001 From: Timm Preetz Date: Wed, 4 Sep 2024 19:50:23 +0200 Subject: [PATCH] Remove `NavigatableSource` And `MappedNavigatableSource`, as it's just too easy to mistakenly create new instances of such classes over and over (in `build`), instead of using retained instances The downside of this might of course be that interacting with "child navigators" from the parent will now become harder, as you can not just call instance methods if you had a reference. (Though this can be fixed by using controllers, ref-like access to the state, or similar patterns.) --- example/lib/main.dart | 4 +- .../src/examples/dynamic_pop_navigator.dart | 26 +++++----- .../examples/example_selection_navigator.dart | 38 +++++--------- .../hot_reloadable_mapped_navigator.dart | 39 --------------- .../src/examples/list_detail_navigator.dart | 30 ++++++++---- lib/fdr.dart | 3 -- .../declarative_navigatable.dart | 1 - .../mapped_navigatable_source.dart | 49 ------------------- .../navigatable_source.dart | 6 --- .../stateful_navigator.dart | 4 +- lib/src/declarative_navigator.dart | 49 ------------------- lib/src/navigatable_to_page_mapper.dart | 26 ---------- test/dynamic_back_button_test.dart | 31 ++++++------ 13 files changed, 67 insertions(+), 239 deletions(-) delete mode 100644 example/lib/src/examples/hot_reloadable_mapped_navigator.dart delete mode 100644 lib/src/declarative_navigatable/mapped_navigatable_source.dart delete mode 100644 lib/src/declarative_navigatable/navigatable_source.dart diff --git a/example/lib/main.dart b/example/lib/main.dart index dc235e0..371ac3e 100644 --- a/example/lib/main.dart +++ b/example/lib/main.dart @@ -34,8 +34,8 @@ class MyHomePage extends StatefulWidget { class _MyHomePageState extends State { @override Widget build(BuildContext context) { - return DeclarativeNavigator.managing( - navigatorFactory: () => ExampleSelectionNavigator(), + return DeclarativeNavigator( + navigator: ExampleSelectionNavigator(), ); } } diff --git a/example/lib/src/examples/dynamic_pop_navigator.dart b/example/lib/src/examples/dynamic_pop_navigator.dart index 893b251..526d42a 100644 --- a/example/lib/src/examples/dynamic_pop_navigator.dart +++ b/example/lib/src/examples/dynamic_pop_navigator.dart @@ -2,17 +2,19 @@ import 'package:fdr/fdr.dart'; import 'package:flutter/cupertino.dart'; import 'package:flutter/material.dart'; -class DynamicPopNavigator extends MappedNavigatableSource< - ( - bool /* shows child */, - bool /* can pop */, - )> { - DynamicPopNavigator() : super(initialState: (true, false)); +class DynamicPopNavigator extends StatefulNavigator { + @override + StatefulNavigatorState createState() => + _DynamicPopNavigatorState(); +} + +class _DynamicPopNavigatorState + extends StatefulNavigatorState { + bool showChild = true; + bool canPop = false; @override List build() { - final canPop = state.$2; - return [ Scaffold( appBar: AppBar( @@ -20,16 +22,16 @@ class DynamicPopNavigator extends MappedNavigatableSource< ), body: Center( child: FilledButton( - onPressed: () => state = (true, false), + onPressed: () => setState(() => showChild = true), child: const Text('Open child page'), ), ), ).page(onPop: null), - if (state.$1) + if (showChild) PopToggle( value: canPop, - onChange: (v) => state = (true, v), - ).page(onPop: canPop ? () => state = (false, false) : null), + onChange: (v) => setState(() => canPop = v), + ).page(onPop: canPop ? () => setState(() => showChild = false) : null), ]; } } diff --git a/example/lib/src/examples/example_selection_navigator.dart b/example/lib/src/examples/example_selection_navigator.dart index 776214f..32a5b83 100644 --- a/example/lib/src/examples/example_selection_navigator.dart +++ b/example/lib/src/examples/example_selection_navigator.dart @@ -1,30 +1,24 @@ import 'package:fdr/fdr.dart'; import 'package:fdr_example/src/examples/dynamic_pop_navigator.dart'; -import 'package:fdr_example/src/examples/hot_reloadable_mapped_navigator.dart'; import 'package:fdr_example/src/examples/hot_reloadable_stateful_navigator.dart'; import 'package:fdr_example/src/examples/hot_reloadable_stateless_navigator.dart'; import 'package:fdr_example/src/examples/list_detail_navigator.dart'; import 'package:fdr_example/src/examples/overlay_portal_navigator.dart'; import 'package:flutter/cupertino.dart'; -class ExampleSelectionNavigator - extends MappedNavigatableSource { - ExampleSelectionNavigator() : super(initialState: null); - - // Manage state, such that any previous value is always cleaned up +class ExampleSelectionNavigator extends StatefulNavigator { @override - set state(DeclarativeNavigatable? newState) { - final state = this.state; - if (state is DisposableNavigatable) { - state.dispose(); - } + StatefulNavigatorState createState() => + _ExampleSelectionNavigatorState(); +} - super.state = newState; - } +class _ExampleSelectionNavigatorState + extends StatefulNavigatorState { + DeclarativeNavigatable? child; @override List build() { - final state = this.state; + final child = this.child; return [ ExampleSelectionPage( @@ -33,22 +27,16 @@ class ExampleSelectionNavigator 'Dynamic back behavior': () => DynamicPopNavigator(), 'Stateful Navigator': () => HotReloadableStatefulNavigator(), 'Stateless Navigator': () => HotReloadableStatelessNavigator(), - 'Mapped Navigator': () => HotReloadableMappedNavigator(), 'Overlay Portal': () => OverlayPortalNavigator(), }, - onExampleSelect: (exampleFactory) => this.state = exampleFactory(), + onExampleSelect: (exampleFactory) => setState( + () => this.child = exampleFactory(), + ), ).page(onPop: null), - if (state != null) state.poppable(onPop: () => this.state = null) + if (child != null) + child.poppable(onPop: () => setState(() => this.child = null)), ]; } - - @override - void dispose() { - // clean up any potentially created navigators - state = null; - - super.dispose(); - } } class ExampleSelectionPage extends StatelessWidget { diff --git a/example/lib/src/examples/hot_reloadable_mapped_navigator.dart b/example/lib/src/examples/hot_reloadable_mapped_navigator.dart deleted file mode 100644 index 0ce2034..0000000 --- a/example/lib/src/examples/hot_reloadable_mapped_navigator.dart +++ /dev/null @@ -1,39 +0,0 @@ -import 'package:fdr/fdr.dart'; -import 'package:flutter/material.dart'; - -class HotReloadableMappedNavigator extends StatelessNavigator { - @override - List build() { - return [ - MappedNavigatorDemo(), - ]; - } -} - -class MappedNavigatorDemo extends MappedNavigatableSource { - MappedNavigatorDemo() : super(initialState: 0); - - @override - List build() { - return [ - Scaffold( - appBar: AppBar( - title: const Text('Stateless Navigator'), - ), - body: Center( - child: Column( - children: [ - Text('Count: $state'), - FilledButton( - onPressed: () { - state++; - }, - child: const Text('Increment'), - ) - ], - ), - ), - ).page(onPop: null), - ]; - } -} diff --git a/example/lib/src/examples/list_detail_navigator.dart b/example/lib/src/examples/list_detail_navigator.dart index 3087482..5dee536 100644 --- a/example/lib/src/examples/list_detail_navigator.dart +++ b/example/lib/src/examples/list_detail_navigator.dart @@ -2,24 +2,36 @@ import 'package:fdr/fdr.dart'; import 'package:flutter/cupertino.dart'; import 'package:flutter/material.dart'; -class ListDetailNavigator extends MappedNavigatableSource { - ListDetailNavigator() : super(initialState: null); +class ListDetailNavigator extends StatefulNavigator { + ListDetailNavigator(); + + @override + StatefulNavigatorState createState() => + _ListDetailNavigatorState(); +} + +class _ListDetailNavigatorState + extends StatefulNavigatorState { + int? selectedNumber; @override List build() { - final state = this.state; + final selectedNumber = this.selectedNumber; + + clear() => setState(() => this.selectedNumber = null); return [ NumberSelectionPage( - onNumberSelect: (number) => this.state = number, + onNumberSelect: (number) => + setState(() => this.selectedNumber = number), ).page(onPop: null), - if (state != null) - if (state == 7) + if (selectedNumber != null) + if (selectedNumber == 7) LuckyNumberSevenPage( - onTap: () => this.state = null, - ).popup(onPop: () => this.state = null) + onTap: clear, + ).popup(onPop: clear) else - NumberDetailPage(number: state).page(onPop: () => this.state = null), + NumberDetailPage(number: selectedNumber).page(onPop: clear), ]; } } diff --git a/lib/fdr.dart b/lib/fdr.dart index 8d9ea8f..9cdba34 100644 --- a/lib/fdr.dart +++ b/lib/fdr.dart @@ -5,15 +5,12 @@ export './src/declarative_navigatable/declarative_navigatable.dart' show DeclarativeNavigatable, DeclarativeNavigatablePage, - NavigatableSource, PageBuilder, Poppable, StatefulNavigator, StatefulNavigatorState, DisposableNavigatable, PopOverwriteNavigatable; -export './src/declarative_navigatable/mapped_navigatable_source.dart' - show MappedNavigatableSource; export './src/declarative_navigatable/stateless_navigator.dart' show StatelessNavigator; export './src/declarative_navigator.dart' show DeclarativeNavigator; diff --git a/lib/src/declarative_navigatable/declarative_navigatable.dart b/lib/src/declarative_navigatable/declarative_navigatable.dart index b0a0540..4ae905b 100644 --- a/lib/src/declarative_navigatable/declarative_navigatable.dart +++ b/lib/src/declarative_navigatable/declarative_navigatable.dart @@ -5,7 +5,6 @@ import 'package:flutter/widgets.dart'; part 'declarative_navigatable_page.dart'; part 'disposable_navigatable.dart'; -part 'navigatable_source.dart'; part 'pop_overwrite_navigatable.dart'; part 'stateful_navigator.dart'; diff --git a/lib/src/declarative_navigatable/mapped_navigatable_source.dart b/lib/src/declarative_navigatable/mapped_navigatable_source.dart deleted file mode 100644 index 466dc8d..0000000 --- a/lib/src/declarative_navigatable/mapped_navigatable_source.dart +++ /dev/null @@ -1,49 +0,0 @@ -import 'package:fdr/fdr.dart'; -import 'package:fdr/src/navigatable_to_page_mapper.dart'; -import 'package:flutter/foundation.dart'; - -abstract class MappedNavigatableSource - implements NavigatableSource, DisposableNavigatable { - MappedNavigatableSource({ - required T initialState, - }) : _state = initialState { - _update(); - - DeclarativeNavigator.hotReload.addListener(_update); - } - - final _pageMapper = NavigatableToPageMapper(); - - T _state; - - @protected - T get state { - return _state; - } - - @protected - set state(T value) { - _state = value; - - _update(); - } - - void _update() { - _pageMapper.updatePages(build()); - } - - @override - ValueListenable> get pages => - _pageMapper.pages; - - @protected - List build(); - - @override - @mustCallSuper - void dispose() { - DeclarativeNavigator.hotReload.removeListener(_update); - - _pageMapper.dispose(); - } -} diff --git a/lib/src/declarative_navigatable/navigatable_source.dart b/lib/src/declarative_navigatable/navigatable_source.dart deleted file mode 100644 index c442698..0000000 --- a/lib/src/declarative_navigatable/navigatable_source.dart +++ /dev/null @@ -1,6 +0,0 @@ -part of 'declarative_navigatable.dart'; - -// When implementing this class, consider whether `pages` should be updated with hot reload -abstract class NavigatableSource implements DeclarativeNavigatable { - ValueListenable> get pages; -} diff --git a/lib/src/declarative_navigatable/stateful_navigator.dart b/lib/src/declarative_navigatable/stateful_navigator.dart index 5d898c3..9c944d7 100644 --- a/lib/src/declarative_navigatable/stateful_navigator.dart +++ b/lib/src/declarative_navigatable/stateful_navigator.dart @@ -4,13 +4,11 @@ abstract class StatefulNavigator implements DeclarativeNavigatable { StatefulNavigatorState createState(); } -abstract class StatefulNavigatorState - implements NavigatableSource { +abstract class StatefulNavigatorState { late T navigator; late final NavigatableToPageMapper _pageMapper; - @override ValueListenable> get pages => _pageMapper.pages; diff --git a/lib/src/declarative_navigator.dart b/lib/src/declarative_navigator.dart index 86ddb43..98d80f0 100644 --- a/lib/src/declarative_navigator.dart +++ b/lib/src/declarative_navigator.dart @@ -9,16 +9,6 @@ class DeclarativeNavigator extends StatefulWidget { required this.navigator, }); - static Widget managing({ - Key? key, - required ValueGetter navigatorFactory, - }) { - return _ManagingDeclarativeNavigator( - key: key, - navigatorFactory: navigatorFactory, - ); - } - static final _hotReload = ChangeNotifier(); static Listenable get hotReload => _hotReload; @@ -80,42 +70,3 @@ class _DeclarativeNavigatorState extends State { super.dispose(); } } - -class _ManagingDeclarativeNavigator extends StatefulWidget { - const _ManagingDeclarativeNavigator({ - super.key, - required this.navigatorFactory, - }); - - final ValueGetter navigatorFactory; - - @override - State<_ManagingDeclarativeNavigator> createState() => - __ManagingDeclarativeNavigatorState(); -} - -class __ManagingDeclarativeNavigatorState - extends State<_ManagingDeclarativeNavigator> { - late final DisposableNavigatable navigator; - - @override - void initState() { - super.initState(); - - navigator = widget.navigatorFactory(); - } - - @override - Widget build(BuildContext context) { - return DeclarativeNavigator( - navigator: navigator, - ); - } - - @override - void dispose() { - navigator.dispose(); - - super.dispose(); - } -} diff --git a/lib/src/navigatable_to_page_mapper.dart b/lib/src/navigatable_to_page_mapper.dart index e97986b..9b43173 100644 --- a/lib/src/navigatable_to_page_mapper.dart +++ b/lib/src/navigatable_to_page_mapper.dart @@ -20,12 +20,6 @@ final class NavigatableToPageMapper { // Only passed when we switch to a different set of navigatables (though some children might overlap) final List? oldNavigatable, ]) { - if (oldNavigatable != null) { - for (final n in oldNavigatable.whereType()) { - n.pages.removeListener(_rebuildPages); - } - } - final pages = []; final states = List.filled( @@ -46,22 +40,6 @@ final class NavigatableToPageMapper { pages.add(item); } - case NavigatableSource(): - item.pages.removeListener(_rebuildPages); - - if (onPopOverwrite != null) { - final childPages = item.pages.value; - - pages.addAll([ - childPages.first.withOnPop(onPop: onPopOverwrite), - ...childPages.skip(1), - ]); - } else { - pages.addAll(item.pages.value); - } - - item.pages.addListener(_rebuildPages); - case StatefulNavigator(): if (_activeStatesByIndex.length > index && _activeStatesByIndex[index]?.isForNavigator(item) == true) { @@ -142,9 +120,5 @@ final class NavigatableToPageMapper { for (final state in _activeStatesByIndex.nonNulls) { state.dispose(); } - - for (final n in _navigatables.whereType()) { - n.pages.removeListener(_rebuildPages); - } } } diff --git a/test/dynamic_back_button_test.dart b/test/dynamic_back_button_test.dart index 9408b7a..2bc6077 100644 --- a/test/dynamic_back_button_test.dart +++ b/test/dynamic_back_button_test.dart @@ -16,8 +16,8 @@ void main() { debugDefaultTargetPlatformOverride = TargetPlatform.android; await tester.pumpWidgetBuilder( - DeclarativeNavigator.managing( - navigatorFactory: () => _Navigator(), + DeclarativeNavigator( + navigator: _Navigator(), ), surfaceSize: size, ); @@ -55,8 +55,8 @@ void main() { debugDefaultTargetPlatformOverride = TargetPlatform.iOS; await tester.pumpWidgetBuilder( - DeclarativeNavigator.managing( - navigatorFactory: () => _Navigator(), + DeclarativeNavigator( + navigator: _Navigator(), ), surfaceSize: size, wrapper: (child) => CupertinoApp( @@ -125,13 +125,14 @@ class _PlatformScaffold extends StatelessWidget { } } -class _Navigator extends MappedNavigatableSource< - ( - bool /* show child */, - bool - /* can pop */ - )> { - _Navigator() : super(initialState: (true, false)); +class _Navigator extends StatefulNavigator { + @override + StatefulNavigatorState<_Navigator> createState() => _NavigatorState(); +} + +class _NavigatorState extends StatefulNavigatorState<_Navigator> { + bool showChild = true; + bool canPop = false; @override List build() { @@ -140,11 +141,11 @@ class _Navigator extends MappedNavigatableSource< title: Text('Home'), child: ColoredBox(color: Colors.blue), ).page(onPop: null), - if (state.$1) + if (showChild) _SwitchPage( - canPop: state.$2, - onCanPopChanged: (v) => state = (true, v), - ).page(onPop: state.$2 ? () => state = (false, false) : null), + canPop: canPop, + onCanPopChanged: (v) => setState(() => canPop = v), + ).page(onPop: canPop ? () => setState(() => showChild = false) : null), ]; } }