fix: 3417 The tagline blinks in the app language, then goes back to English #3426

omkarChend1kar · 2022-12-09T16:23:45Z

What

The tagline blinks in the app language, then goes back to English #3417
When tried debugging, I found that there is flag for checking if snapshot.connectionState == ConnectionState.done, During lifecycle changes when it's in state ConnectionState.waiting it displays default tagline text, Even though snapshot has data.

Fixes bug(s)

Fixes: The tagline blinks in the app language, then goes back to English #3417

monsieurtanuki · 2022-12-09T16:28:44Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

-                if (data.connectionState != ConnectionState.done ||
-                    data.data == null ||
-                    !data.hasData) {
+                if (data.data == null || !data.hasData) {


You probably didn't write the initial code but that is redundant as:

bool get hasData => data != null;

In addition to that, the data.connectionState != ConnectionState.done (or ==, whatever) must be dealt with explicitly, as this is the only thing that really matters in AsyncSnapshot.

You probably didn't write the initial code but that is redundant as:

bool get hasData => data != null;

In addition to that, the data.connectionState != ConnectionState.done (or ==, whatever) must be dealt with explicitly, as this is the only thing that really matters in AsyncSnapshot.

@monsieurtanuki Then we will have to handle ConnectionState.waiting in some other way .let say some sort of loading indicator or shimmer effect, But that too would be just replacing default tagline text with some other thing, Which will cause blinking issue.

Then we will have to handle ConnectionState.waiting in some other way.

@omkarChend1kar I don't understand everything but if you have to, you have to.

It's not just a matter of fixing a bug, it's a matter of making code maintainable. And the first thing I would say if I had to read the code is: "where is done dealt with?".
In addition to that, your specific fix doesn't make any sense in a first approach, as dealing with AsyncSnapshot in the standard lazy way would consider that data.connectionState != ConnectionState.done, data.data == null and !data.hasData are the same thing (which is correct unless the data returned is actually null).

Besides, when it's done it's done, it doesn't look like it can go back to waiting later.

/// Connected to a terminated asynchronous computation. done,

My questions:

are you able to reproduce the bug?

does your fix actually fixes the bug?

would you consider replacing your fix with a more reasonable data.connectionState != ConnectionState.done test?

I have a hunch that the problem comes from the AutoSizeText (a stateFULwidget that gets refreshed) - could you try with a Text instead (a stateLESSwidget)?

@monsieurtanuki I am not sure why it's emitting connectionState.waiting during lifecycle changes, With data being not null whereas documentation suggests otherwise https://api.flutter.dev/flutter/widgets/ConnectionState.html and this is essentially what causing the blinking issue, That's why only thing, I could think was to remove the flag.

Then you're not lucky:

waiting, indicating that the asynchronous operation has begun, typically with the data being null.

Again, I would suggest not to rely on data.hasData (especially as it's unexpectedly populated while still waiting) but only on data.connectionState != ConnectionState.done.
You want to display data when it's complete, don't you?

Then you're not lucky:

waiting, indicating that the asynchronous operation has begun, typically with the data being null.

Again, I would suggest not to rely on data.hasData (especially as it's unexpectedly populated while still waiting) but only on data.connectionState != ConnectionState.done. You want to display data when it's complete, don't you?

@monsieurtanuki Yeah, This is what ideally should be done.

@omkarChend1kar Part of the problem is that you're displaying that within a Consumer that gets refreshed.
Could you add a print statement at the beginning of method _fetchData?
I suspect this method gets called several times, and that would cause the blinking.
If it's not that, you need to investigate "in the literature" in which conditions done can be followed by waiting.

@omkarChend1kar Part of the problem is that you're displaying that within a Consumer that gets refreshed. Could you add a print statement at the beginning of method _fetchData? I suspect this method gets called several times, and that would cause the blinking.

Yeah, It's quite possible, Will check, Thank You @monsieurtanuki!!

If it's not that, you need to investigate "in the literature" in which conditions done can be followed by waiting.

If so, a solution would be to wait until done, to store the data in a private field and to display it.
If refreshed, we should still display the private field until the next done where the private field value would be replaced.
That said, I also suggest that you use a different language (if you don't already), as part of the PR is about "then goes back to English".

@monsieurtanuki Yes indeed ,You are correct, Consumer is rebuilding entire FutureBuilder.

monsieurtanuki

Hi @omkarChend1kar!
Please have a look at my comments; from what I've understood:

we don't need the Consumer at all, and getting rid of it would simply fix the bug (just do that and you're done)
we should not call again and again fetchData, or just once or from time to time and store the result for the current locale (I think you should create a low priority issue for that)

monsieurtanuki · 2022-12-09T18:50:00Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

+                ///This is to avoid blinking issue caused, Due to [ConnectionState.done] followed by [ConnectionState.waiting]
+                ///When [Consumer] refreshes the [FutureBuilder] causing [_fetchData()] to be called again.


As far as I've understood things, this description is a bit misleading: done is never followed by waiting inside the same AsyncSnapshot. The Consumer does call again _fetchData, and that is what should be said first.

monsieurtanuki · 2022-12-09T18:51:11Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

-                if (data.data![TAG_LINE_KEY] != null) {
+                ///This is to avoid blinking issue caused, Due to [ConnectionState.done] followed by [ConnectionState.waiting]
+                ///When [Consumer] refreshes the [FutureBuilder] causing [_fetchData()] to be called again.
+                if (tagline != null) {


You should not store a TagLineItem, but a Map<String, dynamic> instead, because you're currently missing the _SearchCardTagLineDeprecatedAppText case.

Just had a deeper look at fetchData: you should not call it again if the locale did not change (which is very likely). it makes absolutely no sense to call the server again and again.

I have good news: you probably don't need the Consumer at all, as the only added value (?) it provides is setting a default value in one specific case. And the default value in the other cases is also the same value.

I have good news: you probably don't need the Consumer at all, as the only added value (?) it provides is setting a default value in one specific case. And the default value in the other cases is also the same value.

@monsieurtanuki I wasn't able to see the tagline untill, I did my first scan.I guess we will need UserPreferences instance to check the same.

@omkarChend1kar Actually you're right (I won't discuss here the functional added value of waiting for the first scan).
That said, we may agree that

Oops...

@omkarChend1kar Looking again at the code, this is what I would suggest:

replace the Consumer<UserPreferences> syntax with context.watch<UserPreferences>() - not because it's smarter but because it is consistent with the rest of the app, for maintenance purposes

run the fetchData only once by storing the Future in a variable, like we do in main.dart

// We store the argument of FutureBuilder to avoid re-initialization on // subsequent builds. This enables hot reloading. See // https://github.com/openfoodfacts/smooth-app/issues/473 late Future<void> _initFuture; @override void initState() { super.initState(); _initFuture = _init2(); } Future<bool> _init2() async { //... } @override Widget build(BuildContext context) { return FutureBuilder<void>( future: _initFuture, ...

@monsieurtanuki Done!

codecov-commenter · 2022-12-09T20:00:01Z

Codecov Report

Merging #3426 (f3342a5) into develop (69f902e) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #3426      +/-   ##
===========================================
- Coverage    11.30%   11.29%   -0.01%     
===========================================
  Files          260      260              
  Lines        12557    12561       +4     
===========================================
  Hits          1419     1419              
- Misses       11138    11142       +4

Impacted Files	Coverage Δ
...mooth_app/lib/widgets/smooth_product_carousel.dart	`2.45% <0.00%> (-0.07%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…into fix/3417

… into fix/3417

monsieurtanuki

Thank you @omkarChend1kar for your rewriting, that looks much easier to read now!

Removed the flag to check if connection state is not done.

ceec6f7

omkarChend1kar requested a review from a team as a code owner December 9, 2022 16:23

github-actions bot assigned omkarChend1kar Dec 9, 2022

omkarChend1kar changed the title ~~fix: #3417 The tagline blinks in the app language, then goes back to English~~ fix: 3417 The tagline blinks in the app language, then goes back to English Dec 9, 2022

monsieurtanuki reviewed Dec 9, 2022

View reviewed changes

Changes suggested in PR review

f31c1f6

omkarChend1kar requested a review from monsieurtanuki December 9, 2022 18:48

monsieurtanuki reviewed Dec 9, 2022

View reviewed changes

Merge branch 'develop' into fix/3417

7f50df0

teolemon added product scan carousel tagline labels Dec 10, 2022

omkarChend1kar added 3 commits December 12, 2022 09:30

Changes suggested in PR review

fc2dc7b

Merge branch 'develop' of https://github.com/omkarChend1kar/smooth-app …

e54e09b

…into fix/3417

Merge branch 'fix/3417' of https://github.com/omkarChend1kar/smooth-app…

f3342a5

… into fix/3417

omkarChend1kar requested a review from monsieurtanuki December 12, 2022 04:19

monsieurtanuki approved these changes Dec 12, 2022

View reviewed changes

monsieurtanuki merged commit 1301896 into openfoodfacts:develop Dec 12, 2022

github-actions bot mentioned this pull request Dec 12, 2022

chore(develop): release 4.2.0 #3422

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: 3417 The tagline blinks in the app language, then goes back to English #3426

fix: 3417 The tagline blinks in the app language, then goes back to English #3426

omkarChend1kar commented Dec 9, 2022

monsieurtanuki Dec 9, 2022

omkarChend1kar Dec 9, 2022

monsieurtanuki Dec 9, 2022

omkarChend1kar Dec 9, 2022

monsieurtanuki Dec 9, 2022

omkarChend1kar Dec 9, 2022

monsieurtanuki Dec 9, 2022

omkarChend1kar Dec 9, 2022

monsieurtanuki Dec 9, 2022

omkarChend1kar Dec 9, 2022

monsieurtanuki left a comment

monsieurtanuki Dec 9, 2022

monsieurtanuki Dec 9, 2022

monsieurtanuki Dec 9, 2022

monsieurtanuki Dec 9, 2022

omkarChend1kar Dec 10, 2022

monsieurtanuki Dec 10, 2022

monsieurtanuki Dec 10, 2022

omkarChend1kar Dec 12, 2022

codecov-commenter commented Dec 9, 2022 •

edited

Loading

monsieurtanuki left a comment

		///This is to avoid blinking issue caused, Due to [ConnectionState.done] followed by [ConnectionState.waiting]
		///When [Consumer] refreshes the [FutureBuilder] causing [_fetchData()] to be called again.

fix: 3417 The tagline blinks in the app language, then goes back to English #3426

fix: 3417 The tagline blinks in the app language, then goes back to English #3426

Conversation

omkarChend1kar commented Dec 9, 2022

What

Fixes bug(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monsieurtanuki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 9, 2022 • edited Loading

Codecov Report

monsieurtanuki left a comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 9, 2022 •

edited

Loading