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

Endless rebuild in product_image_gallery_view.dart #3238

Closed
monsieurtanuki opened this issue Oct 30, 2022 · 4 comments · Fixed by #3241
Closed

Endless rebuild in product_image_gallery_view.dart #3238

monsieurtanuki opened this issue Oct 30, 2022 · 4 comments · Fixed by #3241
Assignees
Labels
🐛 bug Something isn't working

Comments

@monsieurtanuki
Copy link
Contributor

What

While debugging, I added a print in the build method of product_image_gallery_view.dart, and noticed that it was constantly refreshed (like several times per second).

Steps to reproduce the behavior

  1. Go to product_image_gallery_view.dart
  2. Add a print at the start of the build method
  3. In the app, go to the product image gallery
  4. See in the logs that the print is constantly called

Expected behavior

During the init the print may appear several times, but not forever.

Why

In the build some setStates are called asyncly, which means that the build calls itself again and again.
I cannot predict how bad it is, but basically - beyond the performances that are not an issue in this page AFAIK - it means that never have reliable fields, as they're constantly being refreshed (probably with the same values, though).

@monsieurtanuki monsieurtanuki added the 🐛 bug Something isn't working label Oct 30, 2022
@monsieurtanuki
Copy link
Contributor Author

Additional thoughts:

  • it looks like the product is being downloaded again and again during the refresh, which is not cool
  • it looks like as the fields are not 100% stable, sometimes the app crashes, because when you take a new picture each picture should be either "selected" or "unselected" in the gallery, and you don't know it yet for the new picture as the related computations are async

@monsieurtanuki monsieurtanuki self-assigned this Oct 30, 2022
@monsieurtanuki
Copy link
Contributor Author

@AshAman999 I think the bug comes from your #3023:

  • the async code used to be called once, in initState
  • you moved that async code inside build - the widget is therefore refreshed in an endless loop
  • I think your goal was to have fresh data - in that case it cannot be fresher :)

But beyond that endless loop, there are some things I'd like to understand better.

On that screen, there are 4 main images, one "other" empty image, then all images.

The "other" image is by construction ALWAYS empty: shouldn't we get rid of it, in favor of an "add picture" button?

For some historical reason, the IMAGES field is not part of what we fetch for a product. That means we need to run a specific async server query just for IMAGES: wouldn't we better off including that field into the standard product fields?

It looks like the only thing we can do about a picture on that screen is to (re)take a picture and crop it:

  • we cannot remove a picture
  • we cannot affect an "other" picture to a "main" picture (like "ingredients").

That means that "other" pictures are in a specific zone of "just in case" pictures that cannot be promoted to the "main" zone.
Even on the brand new website I cannot see anymore the tons of pictures already taken about a product. I cannot see ANY picture at all in write mode btw, but that's another story.

My question is: does that make sense to include ALL pictures instead of using only the MAIN pictures?
If it's only a nice-to-have, maybe we could move them to another page, in a next issue/PR.
Having to call again and again the server for pictures that we can edit but that we can't affect to MAIN categories is a bit confusing for the user and annoying for the developer. We would probably be better off with just the main 4 images.
@AshAman999 @teolemon @VaiTon @M123-dev @g123k Your views on the subject please.

@AshAman999
Copy link
Member

AshAman999 commented Oct 31, 2022

Hello @monsieurtanuki
Sorry for the in-intended outcome from #3023, The endless loop I guess it got left undetected by both during the pr that time.

I Agree with you, loading all the pics by default only creates the overhead of API calls, we were better with the 4, for others, it would make another screen to do so,

@monsieurtanuki
Copy link
Contributor Author

Sorry for the in-intended outcome from #3023, The endless loop I guess it got left undetected by both during the pr that time.

Oh my point was not to say your code was wrong, my point was to check if my understanding was correct (in order to fix the bug) and to get your opinion on my other questions (as you've already coded on this page, you may already have exchanged on the "picture situation").

Thank you for your additional answer, as you confirm that it would be more user-friendly to show only the main 4, and perhaps later to show all pictures in an additional page, if needed.

monsieurtanuki added a commit to monsieurtanuki/smooth-app that referenced this issue Oct 31, 2022
…ust the main 4)

Impacted files:
* `product_cards_helper.dart`: added an optional parameter in order to ignore "other" pictures
* `product_image_gallery_view.dart`: removed the "other" pictures
monsieurtanuki added a commit to monsieurtanuki/smooth-app that referenced this issue Nov 3, 2022
monsieurtanuki added a commit that referenced this issue Nov 3, 2022
…4) (#3241)

Impacted files:
* `product_cards_helper.dart`: added an optional parameter in order to ignore "other" pictures
* `product_image_gallery_view.dart`: removed the "other" pictures
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Development

Successfully merging a pull request may close this issue.

2 participants