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

feat: Instant refresh views #2901

Merged

Conversation

AshAman999
Copy link
Member

@AshAman999 AshAman999 commented Sep 1, 2022

What

Instantly update views on product edit

Steps

  • Go to the edit page of any product
  • Edit the basic details like name, brand name, etc ...
  • go back to the previous screen, then again to the same edit page
  • the changes have not been updated

This pr would fix those problems

  • would immediately change the product state in the edit screen
  • works even when offline
  • Immediate response compared to what was earlier after the offline pr merge

Screen Recording

20220905_011032.mp4

Part of

@AshAman999 AshAman999 requested a review from a team as a code owner September 1, 2022 20:57
@AshAman999 AshAman999 marked this pull request as draft September 1, 2022 20:58
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @AshAman999!
I'm just commenting, as it's just a draft.
I'm not sure your description of the PR is correct: on an existing product, if you go on the big edit page, change the category, go back to the big edit page and go to category, this works as expected: the category you chose earlier is pre-selected, right?
I suspect the problem is with new products, where (maybe) the product is not immediately saved on the server (and therefore refreshed locally) before all mandatory fields are input (which may not be done in one single edit, I don't know).
One problem here is probably a mix between the offline mode (with its tasks) and the normal mode in the rest of the app (we call the server, the server saves the product, we refresh from the server, the consumer/provider is called, the product data is refreshed in the whole app).
It would be safe to build the offline mode / task on top of something that works. Mixing both is perhaps confusing.
My suggestion: make it work ignoring the offline mode (cf. EditProductPage), then make it work with the offline mode on top.

@abughalib
Copy link
Contributor

@monsieurtanuki
I think this is what Aman's PR is about.

Record_2022-09-02-18-02-25.mp4

@monsieurtanuki
Copy link
Contributor

@abughalib This issue does not happen on the other edit detail pages, like "edit categories".
That's why I suggest to apply a code similar to what is implemented in "edit categories" regarding saving the data (and refreshing it locally).
Which won't work on offline mode.
And then, once the whole product editions work in online mode, find a way to make it work in a downgraded offline mode.

@AshAman999
Copy link
Member Author

One problem here is probably a mix between the offline mode (with its tasks) and the normal mode in the rest of the app (we call the server, the server saves the product, we refresh from the server, the consumer/provider is called, the product data is refreshed in the whole app).

Bonjour @monsieurtanuki

Yeah agreed, some things were working fine earlier as you're suggesting
But since the last merge for keeping simple tasks in background like these edits and uploading images, What's actually happening is we are doing the task of calling the API and updating the database in background, and
I can't access context there, so can't set the state there in the background.

My solution was to, as we already know what fields we change
https://github.com/openfoodfacts/smooth-app/blob/develop/packages/smooth_app/lib/pages/product/nutrition_page_loaded.dart

    final Product? product = await daoProduct.get(
      _product.barcode!,
    );
    // We go and chek in the local database if the product is
    // already in the database. If it is, we update the fields of the product.
    //And if it is not, we create a new product with the fields of the _product
    // and we insert it in the database. (Giving the user an immediate feedback)
    if (product != null) {
      product.servingSize = changedProduct.servingSize;
      product.nutriments = changedProduct.nutriments;
      await daoProduct.put(product);
    }
    localDatabase.notifyListeners();
    if (mounted) {
      ScaffoldMessenger.of(context).showSnackBar(
        SnackBar(
          content: Text(appLocalizations.product_task_background_schedule),
        ),
      );
    }
    return true;
  }
}

We can, we just update the product in the local database and the change the set/view with provider accordingly so in case if our queries take time to be done (and also I can't update state from background) it gives the user immediate results that at least their changes did happen, and it appears also.

I am just stuck on the lib\pages\product\simple_input_page.dart pages to implement something where I could just get the changes, and create a product which is merge of the changed product and the cached local database product. This is where I was stuck and that's why I left the pr as draft, any helps would be appreciated

I Hope this clears some confusion about what this pr was intended for,

@monsieurtanuki
Copy link
Contributor

Hi @AshAman999!
I think I understood most of the situation here.
My suggestion is still the same: first do something that works (cf. SimpleInputPage), and in a next PR do something that would work offline too. I can help you with that next PR, which would be something like an upgrade of saveAndRefresh.

I can't access context there, so can't set the state there in the background.

So don't use setState then. Maybe notifyListeners inside the isolate? Maybe notifyListeners outside the isolate when it returns?
But first, do something that works online.

@AshAman999 AshAman999 force-pushed the instant-product-refresh-on-save branch from 8d36827 to 3b383f1 Compare September 2, 2022 16:13
@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2022

Codecov Report

Merging #2901 (e73f7a9) into develop (d9c35d7) will increase coverage by 0.17%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           develop   #2901      +/-   ##
==========================================
+ Coverage     6.71%   6.88%   +0.17%     
==========================================
  Files          228     229       +1     
  Lines        11117   11138      +21     
==========================================
+ Hits           746     767      +21     
  Misses       10371   10371              
Impacted Files Coverage Δ
...h_app/lib/cache/files/file_cache_manager_impl.dart 94.59% <ø> (ø)
...pp/lib/pages/onboarding/onboarding_bottom_bar.dart 0.00% <0.00%> (ø)
..._app/lib/pages/product/add_basic_details_page.dart 0.00% <0.00%> (ø)
...h_app/lib/pages/product/edit_ingredients_page.dart 0.00% <0.00%> (ø)
...smooth_app/lib/pages/product/new_product_page.dart 0.00% <0.00%> (ø)
...oth_app/lib/pages/product/nutrition_container.dart 0.00% <0.00%> (ø)
...h_app/lib/pages/product/nutrition_page_loaded.dart 0.00% <0.00%> (ø)
..._app/lib/pages/product/ocr_ingredients_helper.dart 0.00% <0.00%> (ø)
...th_app/lib/pages/product/ocr_packaging_helper.dart 0.00% <0.00%> (ø)
...mooth_app/lib/pages/product/simple_input_page.dart 0.00% <0.00%> (ø)
... and 42 more

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

@AshAman999 AshAman999 marked this pull request as ready for review September 4, 2022 19:43
@AshAman999
Copy link
Member Author

Hello @monsieurtanuki

For something that actually listens to the change from the database on that screen, I thought of other ideas
Currently, you make a change, and as soon as you exit the page the background process for that task is running and when it finishes it fetches the fresh product and saves it into the database.
Since the database product is already updated, what probably could be done is running a periodic function of maybe a 5-second delay on edit_product_page.dart that would fetch the latest product from the database and update the view there.

Surely there can be better options than running periodic function, but for now, this is the only solution I was able to come up with, I could create a pr for that if you say,

Meanwhile, this pr would solve some of the problems for now

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @AshAman999 for having applied the suggested changes to SimpleInputPage.
If you could apply the same kind of changes in the other 3 pages that would be perfect for results and maintenance; the key is to have a method that changes the product, that can be applied to a full cached product or a minimalist product.

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AshAman999 We're close!
But I insist that using the same coding pattern for the same actions in the 4 pages makes sense, consistency-wise, effectivity-wise and maintenance-wise.
You'll find my detailed remarks.

@AshAman999
Copy link
Member Author

Bonjour @monsieurtanuki

Phew 😓 , Finally was able to understand the code for nutrient fields,
I hope this would be okay enough
Thanks :)

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @AshAman999 for your work!
In a next PR we will refactor those classes for some consistency (e.g. same getChangedProduct method names), but for the moment it's fine!

@AshAman999 AshAman999 force-pushed the instant-product-refresh-on-save branch from 25214bd to e73f7a9 Compare September 6, 2022 07:48
@teolemon
Copy link
Member

teolemon commented Sep 6, 2022

Ok, it seems work is still ongoing on the PR. Waiting for a while before merging.

@teolemon teolemon added ✏️ Editing Many products are incomplete and don't have Nutri-Score, Eco-Score…so editing is important for users Offline Offline - Contribution labels Sep 6, 2022
@AshAman999
Copy link
Member Author

Ok, it seems work is still ongoing on the PR. Waiting for a while before merging.

It's done, and approved
We can merge it for sure , no problem in that ✅

@VaiTon VaiTon requested review from a team and monsieurtanuki September 6, 2022 10:02
@monsieurtanuki
Copy link
Contributor

I've already reviewed (many many times) this code. I approved it this morning.
Then I think @AshAman999 had an unfortunate action that pushed again exactly the same code. Am I right?
If so, please consider the code approved, and please @AshAman999 avoid this kind of incident, your code could smoothly have been merged.

@AshAman999
Copy link
Member Author

Real Sorry about this @monsieurtanuki

I just rebased the pr on the latest changes, Better be careful about this next time.

@teolemon teolemon merged commit 0d2be11 into openfoodfacts:develop Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Editing Many products are incomplete and don't have Nutri-Score, Eco-Score…so editing is important for users Offline - Contribution Offline
Projects
Development

Successfully merging this pull request may close these issues.

5 participants