Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Localization support #49

Closed
1 task done
t-girniak opened this issue May 10, 2022 · 6 comments
Closed
1 task done

Localization support #49

t-girniak opened this issue May 10, 2022 · 6 comments

Comments

@t-girniak
Copy link

Is there an existing feature request for this?

  • I have searched the existing issues.

Command

It would be awesome to add a support for the localisation in the library

Description

First of all, thanks for an awesome library and your work!

The problem is, I haven't found a proper way to add localisations delegate to the golden test, and this feature is extremely important to have in order to test any widget with localisation.

I've added localizationsDelegates param in my fork to enable localisation, and it actually worked. But the problem is, after adding an AppLocalizationDelegate to the MaterialApp, alchemist has stopped rendering any kind of images. Any asset or file image would become transparent. However, after removing the delegate, images appear again. I've tested adding the same AppLocalizationDelegate to the MaterialApp with a golden_toolkit library, and there was no such problem (images were rendered and localisation worked as well).

Maybe you have any ideas on how to add a localizationsDelegates to the test in a way that prevents this issue with rendering images?

Reasoning

I think that supporting localisation in the library would greatly increase the usability

Additional context and comments

You can refer to my closed PR, where I've added the coreWrapper: #43

@Kirpal
Copy link
Collaborator

Kirpal commented May 10, 2022

Have you tried using a Localizations widget? I tried running this test and it worked as expected.

@t-girniak
Copy link
Author

I've tried running your exact test but with Image.asset() and Image.file() widgets along with a text - the image is not rendered if I pass your FakeLocalizationsDelegate to delegates list. The same happens If I pass real delegate but not the fake one.

However, after I removed the FakeLocalizationsDelegate from delegates list - image rendered normally. The problem with localisation in the library is that it can not be combined with images right now.

Try running your asset_image_smoke_test but with Localizations widget as a wrapper in the builder: https://github.com/Betterment/alchemist/blob/main/test/smoke_tests/asset_image_smoke_test.dart

  goldenTest(
    'succeeds with an asset image',
    fileName: 'asset_image_smoke_test',
    pumpBeforeTest: precacheImages,
    builder: () => Localizations(
      locale: const Locale('en'),
      delegates: const [
        DefaultWidgetsLocalizations.delegate,
        FakeLocalizationsDelegate(),
      ],
      child: DefaultAssetBundle(
        bundle: FakeTestAssetBundle(),
        child: Image.asset('test.png'),
      ),
    ),
  );

It won't render the red square 🟥 :
image

But if you run the same test as above but without FakeLocalizationsDelegate() - it will render the red square in golden:

  goldenTest(
    'succeeds with an asset image',
    fileName: 'asset_image_smoke_test',
    pumpBeforeTest: precacheImages,
    builder: () => Localizations(
      locale: const Locale('en'),
      delegates: const [
        DefaultWidgetsLocalizations.delegate,
      ],
      child: DefaultAssetBundle(
        bundle: FakeTestAssetBundle(),
        child: Image.asset('test.png'),
      ),
    ),
  );

image

These tests were run on alchemist v0.3.3

@Kirpal
Copy link
Collaborator

Kirpal commented May 11, 2022

Oh I see, sorry I misinterpreted your issue. Looks like this is being caused by the FakeLocalizationsDelegate loading asynchronously. I was able to get it working in two possible ways:

  1. in the FakeLocalizationsDelegate, make load return synchronously:
    (This may not be desired if you're using your app's actual Localizations)
Future<FakeLocalizations> load(Locale locale) => SynchronousFuture(FakeLocalizations());
  1. add an extra pump to the pumpBeforeTest so it loads the strings before caching the images:
pumpBeforeTest: (tester) async {
  await pumpOnce(tester);
  await precacheImages(tester);
}

Let me know if this solves your issue!

@t-girniak
Copy link
Author

Thanks for your help @Kirpal! It actually solved my issue!

This extra pump actually resolved the issue for me, and I was testing with real localisation, not the fake one :)
It worked out even without making load() synchronous, all thanks to adding await pumpOnce(tester);

I am not 100% sure that this workaround will resolve all of the similar cases, but maybe await pumpOnce(tester); should be added to the precacheImages function directly to prevent this bug? It's not really obvious, and there may be others straggling with this kind of issue.

@Kirpal
Copy link
Collaborator

Kirpal commented May 11, 2022

I'm conflicted about adding it to precache images because it might be unexpected in some cases (while necessary in cases like this). I lean more on the side of not including it because that allows for more precise control of the rendering. If it were to be included in precacheImages it might cause issues with animations for example, and then a user would need to create a copy of precacheImages without the pump to get the rendering they want.

I do think it's probably worth documenting this somewhere though because this case will happen in other widgets with similar rendering behavior to Localizations(deferring the first frame).

@t-girniak
Copy link
Author

Yes, I agree with you. But it is worth documenting this case for sure then. Thanks a lot for your help Kirpal!
I am closing the issue for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants