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

Correct use of provider and notify #2439

Closed
monsieurtanuki opened this issue Jun 30, 2022 · 6 comments
Closed

Correct use of provider and notify #2439

monsieurtanuki opened this issue Jun 30, 2022 · 6 comments
Assignees
Milestone

Comments

@monsieurtanuki
Copy link
Contributor

monsieurtanuki commented Jun 30, 2022

Problem

We experience every day issues regarding products or list not being refreshed.
Most of the time, we just use provider for static fields: let's get the database with context.watch<LocalDatabase>().
And if we notify the LocalDatabase, sometimes it's good (actually it's always good) but sometimes there are side-effects (like not necessary page refreshes that don't look good).

Proposed solution

I think it's time to refactor our use of providers.

If we focus on products, I think we have two strong needs:

  • being notified when a product has changed
  • being notified when a product list has changed
  • (minor need) not being notified for things we don't care

Solutions would include:

  • use of select, in order to say "on this page the rest of the world may change but I only care about this product"
  • understanding on every page what should trigger a refresh
  • find a readable way to code this, so that if we look at the code we immediately see "here we care about that list / that product / we never refresh"

Additional context

Part of

@M123-dev
Copy link
Member

M123-dev commented Jul 2, 2022

💯

Especially the UserPreferences got really fat

@monsieurtanuki
Copy link
Contributor Author

That is what I plan to implement regarding product refresh:

  1. we need stupid Widgets that take a Product as input parameter, like StatelessWidgets - let's call them "A"
  2. upstream of "A" widgets, we need widgets that detect the changes on the product, like StatelessWidgets with provider/select and FutureBuilder/DaoProduct.get - let's call them "B". Typical existing example is the product page. Typical non-existing example is the item in a product list page: we need to embed the current smooth-product-found-card with a FutureBuilder
  3. What will trigger the provider is a new product timestamp. We cannot store the whole Product, it would take too much space
  4. When we start the app, we store no timestamp for the products. It's only when you save a product (DaoProduct.put) that you update the timestamp of the product - which will trigger a refresh of all related "B" widgets therefore "A" widgets.
  5. Given that the refresh is going to be almost instant (get a product from a SQL primary key in a local database), we may not find interesting to display something relevant while the product is loaded. A Container() will be ok in a first approach.

@M123-dev Any remark or suggestion?

@M123-dev
Copy link
Member

M123-dev commented Jul 5, 2022

Not really the plan looks good to me, just to number 5, even though it's nearly instant I would still add a loading indicator in case due to some bad circumstances it takes a few seconds more.

@monsieurtanuki
Copy link
Contributor Author

@M123-dev I agree, though it would be a bit lazy to put a CircularProgressIndicator (not worse than Container I admit). Well, we'll see, that goes a bit beyond that issue: in the case of the product list page instead of watching 25 progress indicators we could see 25 "empty" smooth-product-card-found template (like, in grey), like what you see sometimes in websites when the data is not loaded yet.
The tests IRW will tell.

@M123-dev
Copy link
Member

M123-dev commented Jul 5, 2022

Maybe something like this

"shimmer | Flutter Package" https://pub.dev/packages/shimmer

But as you said that's not a priority in any terms

@monsieurtanuki
Copy link
Contributor Author

Fixed by #3018.

Repository owner moved this from To discuss and validate to Done in 🤳🥫 The Open Food Facts mobile app (Android & iOS) Nov 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants