wiredashio / wiredash-sdk

Interactive user feedback tool for Flutter 🎉
https://pub.dev/packages/wiredash
Other
514 stars 66 forks source link

Causes child widgets to lose state when localization delegate is added #341

Closed saibotma closed 7 months ago

saibotma commented 7 months ago

Describe the bug Causes child widgets to lose state when localization delegate is added. Steps to reproduce:

Example

Expand ```dart import 'package:flutter/material.dart'; import 'package:wiredash/wiredash.dart'; void main() { runApp(const MyApp()); } class MyApp extends StatefulWidget { const MyApp({super.key}); @override State createState() => _MyAppState(); } class _MyAppState extends State { final _options = const WiredashOptionsData( localizationDelegate: CustomWiredashTranslationsDelegate(), ); @override Widget build(BuildContext context) { return Wiredash( projectId: "xxxxx", secret: "xxxxx", options: _options, child: const MaterialApp( home: TestWidget(), ), ); } } class TestWidget extends StatefulWidget { const TestWidget({super.key}); @override State createState() => _TestWidgetState(); } class _TestWidgetState extends State { @override void initState() { super.initState(); print("TestWidget initState"); } @override Widget build(BuildContext context) { return Scaffold( body: const Placeholder(), floatingActionButton: FloatingActionButton( onPressed: () => Wiredash.of(context).show(), ), ); } } class CustomWiredashTranslationsDelegate extends LocalizationsDelegate { const CustomWiredashTranslationsDelegate(); @override bool isSupported(Locale locale) { return ['en'].contains(locale.languageCode); } @override Future load(Locale locale) async { return switch (locale.languageCode) { 'en' => _EnOverrides(), _ => throw "Unsupported locale $locale", }; } @override bool shouldReload(CustomWiredashTranslationsDelegate old) => false; } class _EnOverrides extends WiredashLocalizationsEn { @override String get feedbackStep1MessageHint => 'Test'; } ```

Wiredash SDK Info Version 2.1.0

Flutter SDK Version: 3.19.1

passsy commented 7 months ago

Thanks for the detailed bug report! I was able to reproduce it in the test below.

Test code ```dart import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:wiredash/wiredash.dart'; void main() { group('issue 341', () { testWidgets('with options', (tester) async { await tester.pumpWidget(const MyApp(withDelegate: true)); await tester.pumpAndSettle(); expect(TestWidget.createCount, 1); await tester.tap(find.byType(FloatingActionButton)); await tester.pumpAndSettle(); expect(TestWidget.createCount, 1); }); testWidgets('without options', (tester) async { await tester.pumpWidget(const MyApp(withDelegate: false)); await tester.pumpAndSettle(); expect(TestWidget.createCount, 1); await tester.tap(find.byType(FloatingActionButton)); await tester.pumpAndSettle(); expect(TestWidget.createCount, 1); }); }); } class MyApp extends StatefulWidget { const MyApp({ super.key, required this.withDelegate, }); @override State createState() => _MyAppState(); final bool withDelegate; } class _MyAppState extends State { final _options = const WiredashOptionsData( localizationDelegate: CustomWiredashTranslationsDelegate(), ); @override Widget build(BuildContext context) { return Wiredash( projectId: "xxxxx", secret: "xxxxx", options: widget.withDelegate ? _options : null, child: const MaterialApp( home: TestWidget(), ), ); } } class TestWidget extends StatefulWidget { const TestWidget({super.key}); @override State createState() => _TestWidgetState(); static int createCount = 0; } class _TestWidgetState extends State { @override void initState() { super.initState(); print("TestWidget initState"); addTearDown(() { TestWidget.createCount = 0; }); TestWidget.createCount++; } @override Widget build(BuildContext context) { return Scaffold( body: const Placeholder(), floatingActionButton: FloatingActionButton( onPressed: () => Wiredash.of(context).show(), ), ); } } class CustomWiredashTranslationsDelegate extends LocalizationsDelegate { const CustomWiredashTranslationsDelegate(); @override bool isSupported(Locale locale) { return ['en'].contains(locale.languageCode); } @override Future load(Locale locale) async { switch (locale.languageCode) { case 'en': return _EnOverrides(); default: throw "Unsupported locale $locale"; } } @override bool shouldReload(CustomWiredashTranslationsDelegate old) => false; } class _EnOverrides extends WiredashLocalizationsEn { @override String get feedbackStep1MessageHint => 'Test'; } ```

Root cause

It is a known issue, caused by the Localizations widget. It loads the Locale asynchronous (calling LocalizationsDelegate.load(Locale)). While loading, it returns a SizedBox() instead of the app for roughly 1 frame.

// class Localizations, flutter/lib/src/widgets/localizations.dart
  @override
  Widget build(BuildContext context) {
    if (_locale == null) {
      return const SizedBox.shrink(); // Shown for one frame
    }
    return Semantics(
      textDirection: _textDirection,
      child: _LocalizationsScope(
        key: _localizedResourcesScopeKey,
        locale: _locale!,
        localizationsState: this,
        typeToResources: _typeToResources,
        child: Directionality(
          textDirection: _textDirection,
          child: widget.child!, // <-- this is your app
        ),
      ),
    );
  }

Because your app is removed for one frame, it loses its state.

Workaround

The workaround is rather straight forward. Change your load method to return a SynchronousFuture. Remove the async keyword.

  // Your app will lose state
  @override
-  Future<WiredashLocalizations> load(Locale locale) async {
+  Future<WiredashLocalizations> load(Locale locale) {
    switch (locale.languageCode) {
      case 'en':
-        return _EnOverrides();
+        return SynchronousFuture(_EnOverrides());
      default:
        throw "Unsupported locale $locale";
    }
  }

Wiredash should catch this case, and show a warning when load does not return a SynchronousFuture. Implementations that really load the localizations asynchronously, can asynchronously create a synchronous version of their LocalizationsDelegate instead.

Unfortunately, Wiredash can not yet get rid of the Localizations widget, or replace it with its own implementation. Wiredash still depends on a hand-full of material widgets (like TextFormField) which require the Localizations widget.

saibotma commented 7 months ago

Thanks for the quick response & implementing the warning.