From 8c3d5838dd539fec52731bdce3dc73bcc11c4f7c Mon Sep 17 00:00:00 2001 From: Yegor Date: Fri, 22 Sep 2017 10:49:19 -0700 Subject: [PATCH] Fix inherited widget notifications in Localizations (#12213) * Fix inherited widget notifications in Localizations * address comments --- packages/flutter/lib/src/material/tabs.dart | 2 +- .../flutter/lib/src/widgets/framework.dart | 23 +++++++++++++++ .../lib/src/widgets/localizations.dart | 18 +++++++++--- .../test/material/localizations_test.dart | 29 +++++++++++++++++++ 4 files changed, 67 insertions(+), 5 deletions(-) diff --git a/packages/flutter/lib/src/material/tabs.dart b/packages/flutter/lib/src/material/tabs.dart index 9e38ab161b..28053082f1 100644 --- a/packages/flutter/lib/src/material/tabs.dart +++ b/packages/flutter/lib/src/material/tabs.dart @@ -794,7 +794,7 @@ class _TabBarState extends State { } /// A page view that displays the widget which corresponds to the currently -/// selected tab. Typically used in conjuction with a [TabBar]. +/// selected tab. Typically used in conjunction with a [TabBar]. /// /// If a [TabController] is not provided, then there must be a [DefaultTabController] /// ancestor. diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index 4144cac28f..52aa35b31b 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart @@ -3287,9 +3287,31 @@ abstract class Element extends DiagnosticableTree implements BuildContext { @mustCallSuper void didChangeDependencies() { assert(_active); // otherwise markNeedsBuild is a no-op + _debugCheckOwnerBuildTargetExists('didChangeDependencies'); markNeedsBuild(); } + void _debugCheckOwnerBuildTargetExists(String methodName) { + assert(() { + if (owner._debugCurrentBuildTarget == null) { + throw new FlutterError( + '$methodName for ${widget.runtimeType} was called at an ' + 'inappropriate time.\n' + '\n' + 'It may only be called while the widgets are being built. A possible ' + 'cause of this error is when $methodName is called during ' + 'one of:\n' + '\n' + ' * network I/O event\n' + ' * file I/O event\n' + ' * timer\n' + ' * microtask (caused by Future.then, async/await, scheduleMicrotask)' + ); + } + return true; + }); + } + /// Returns a description of what caused this element to be created. /// /// Useful for debugging the source of an element. @@ -3952,6 +3974,7 @@ class InheritedElement extends ProxyElement { /// by first obtaining their [InheritedElement] using /// [BuildContext.ancestorInheritedElementForWidgetOfExactType]. void dispatchDidChangeDependencies() { + _debugCheckOwnerBuildTargetExists('dispatchDidChangeDependencies'); for (Element dependent in _dependents) { assert(() { // check that it really is our descendant diff --git a/packages/flutter/lib/src/widgets/localizations.dart b/packages/flutter/lib/src/widgets/localizations.dart index 75ab2c51c0..7ac42acaff 100644 --- a/packages/flutter/lib/src/widgets/localizations.dart +++ b/packages/flutter/lib/src/widgets/localizations.dart @@ -206,6 +206,7 @@ class _LocalizationsScope extends InheritedWidget { Key key, @required this.locale, @required this.localizationsState, + @required this.loadGeneration, Widget child, }) : super(key: key, child: child) { assert(localizationsState != null); @@ -214,10 +215,14 @@ class _LocalizationsScope extends InheritedWidget { final Locale locale; final _LocalizationsState localizationsState; + /// A monotonically increasing number that changes after localizations + /// delegates have finished loading new data. When this number changes, it + /// triggers inherited widget notifications. + final int loadGeneration; + @override bool updateShouldNotify(_LocalizationsScope old) { - // Changes in Localizations.locale trigger a load(), see _LocalizationsState.didUpdateWidget() - return false; + return loadGeneration != old.loadGeneration; } } @@ -431,6 +436,11 @@ class _LocalizationsState extends State { final GlobalKey _localizedResourcesScopeKey = new GlobalKey(); Map _typeToResources = {}; + /// A monotonically increasing number that increases after localizations + /// delegates have finished loading new data, triggering inherited widget + /// notifications. + int _loadGeneration = 0; + Locale get locale => _locale; Locale _locale; @@ -494,9 +504,8 @@ class _LocalizationsState extends State { setState(() { _typeToResources = value; _locale = locale; + _loadGeneration += 1; }); - final InheritedElement scopeElement = _localizedResourcesScopeKey.currentContext; - scopeElement?.dispatchDidChangeDependencies(); }); } } @@ -521,6 +530,7 @@ class _LocalizationsState extends State { key: _localizedResourcesScopeKey, locale: _locale, localizationsState: this, + loadGeneration: _loadGeneration, child: new Directionality( textDirection: _textDirection, child: widget.child, diff --git a/packages/flutter/test/material/localizations_test.dart b/packages/flutter/test/material/localizations_test.dart index 8bcd1eb2ec..c27993bac1 100644 --- a/packages/flutter/test/material/localizations_test.dart +++ b/packages/flutter/test/material/localizations_test.dart @@ -25,6 +25,18 @@ class FooMaterialLocalizationsDelegate extends LocalizationsDelegate false; } +/// A localizations delegate that does not contain any useful data, and is only +/// used to trigger didChangeDependencies upon locale change. +class _DummyLocalizationsDelegate extends LocalizationsDelegate { + @override + Future load(Locale locale) async => new DummyLocalizations(); + + @override + bool shouldReload(_DummyLocalizationsDelegate old) => true; +} + +class DummyLocalizations {} + Widget buildFrame({ Locale locale, Iterable> delegates, @@ -293,4 +305,21 @@ void main() { expect(tester.widget(find.byKey(textKey)).data, 'id_JV'); }); + testWidgets('Localizations is compatible with ChangeNotifier.dispose() called during didChangeDependencies', (WidgetTester tester) async { + // PageView calls ScrollPosition.dispose() during didChangeDependencies. + await tester.pumpWidget(new MaterialApp( + supportedLocales: const [ + const Locale('en', 'US'), + const Locale('es', 'ES'), + ], + localizationsDelegates: <_DummyLocalizationsDelegate>[ + new _DummyLocalizationsDelegate(), + ], + home: new PageView(), + )); + + await tester.binding.setLocale('es', 'US'); + await tester.pump(); + await tester.pumpWidget(new Container()); + }); }