feature/#55 - SQLite local database, and refactoring with StatefulWidget's #62

monsieurtanuki · 2021-01-01T20:12:21Z

New files:

barcode_product_query.dart: API query / product by barcode (used to be in full_products_database.dart)
local_database.dart: local SQLite database with one table (product)

Deleted files:

choose_page_model.dart: moved the code to StatefulWidget ChoosePage
full_product_database.dart: database code was deprecated; API fields is now in ProductQuery

Impacted files:

alternative_continuous_scan_page.dart: now a StatefulWidget; refactored with ContinuousScanPage
build.gradle: unrelated - lowered the Android minSdkVersion from 24 to 19, in order to run the app on my old smartphone ;)
choose_page.dart: refactoring as changed to StatefulWidget
continuous_scan_model.dart: not a ChangeNotifier anymore - should be improved when we use product list
continuous_scan_page.dart: now a StatefulWidget
group_product_query.dart: minor refactoring due to the changes in product_query.dart
keywords_product_query.dart: minor refactoring due to the changes in product_query.dart
main.dart: added LocalDatabase to the providers; simplified the code; removed mentions to SharedPreferences
product_query.dart: refactored a bit; moved here some fields from full_product_database.dart
product_query_model.dart: now using LocalDatabase
product_query_page.dart: now using LocalDatabase; refactoring
pubspec.yaml: replaced sembast with sqflite`

…get's New files: * `barcode_product_query.dart`: API query / product by barcode (used to be in `full_products_database.dart`) * `local_database.dart`: local SQLite database with one table (`product`) Deleted files: * `choose_page_model.dart`: moved the code to `StatefulWidget` `ChoosePage` * `full_product_database.dart`: database code was deprecated; API fields is now in `ProductQuery` Impacted files: * `alternative_continuous_scan_page.dart`: now a `StatefulWidget`; refactored with `ContinuousScanPage` * `build.gradle`: unrelated - lowered the Android `minSdkVersion` from `24` to `19`, in order to run the app on my old smartphone ;) * `choose_page.dart`: refactoring as changed to `StatefulWidget` * `continuous_scan_model.dart`: not a `ChangeNotifier` anymore - should be improved when we use product list * `continuous_scan_page.dart`: now a `StatefulWidget` * `group_product_query.dart`: minor refactoring due to the changes in `product_query.dart` * `keywords_product_query.dart`: minor refactoring due to the changes in `product_query.dart` * `main.dart`: added `LocalDatabase` to the providers; simplified the code; removed mentions to `SharedPreferences` * `product_query.dart`: refactored a bit; moved here some fields from `full_product_database.dart` * `product_query_model.dart`: now using `LocalDatabase` * `product_query_page.dart`: now using `LocalDatabase`; refactoring * `pubspec.yaml`: replaced `sembast with `sqflite`

PrimaelQuemerais

Apart from that one comment it looks good, I will pull it to try it myself.
Thank you for the refactoring, some of those were indeed needed.

PrimaelQuemerais · 2021-01-01T20:30:43Z

packages/smooth_app/lib/data_models/continuous_scan_model.dart


 enum ScannedProductState { FOUND, NOT_FOUND, LOADING }

-class ContinuousScanModel extends ChangeNotifier {


Is there a valid reason to get rid of the ChangeNotifier ?
We are using Provider as State Management and the StatefulWidgets should be avoided (Now that I think about it I shouldn't have accepted the new Profile page as it doesn't follow this guideline).

Provider works by separating the UI from the logic, every Widget can be stateless and the class that extends ChangeNotifier holds all the variables, calling notifyListener() tells the Widget to rebuild.

Sorry that this wasn't specified anywhere on the project, I will update the guidelines.

I understand your point about the ChangeNotifier vs. StatefulWidget.
That being said, the code before I refactored it was really painful to read, with Consumers here and there.
My idea is to have most of the relevant data in the Providers that are already in the main.dart - in that case we could notify each time the scan list is updated as a product list stored in the database, for instance.
Interesting point, to be investigated.
Is there a link for the guidelines?

I agree that the use of Consumers is quite heavy but I also fear that mixing ChangeNotifiers and StatefulWidgets can become confusing. Perhaps we should determine a clear rule on when Provider or StatefulWidgets should be used.
Having the data at a higher levels such as main.dart sounds good to me, I don't really see how that will reduce the use of Consumers though.

There is actually no guidelines at the moment, only the technical document you already saw for the choice of database. I will start working on guidelines and documentation for future contributors.

I don't really see how that will reduce the use of Consumers though.

That's the beauty of Providers: if I start my build(BuildContext context) with a context.watch<LocalDatabase>(), every time the notify listener method is called on LocalDatabase, the Widget is rebuilt. And I don't need any Consumer.

Don't put too much effort in the guidelines for the moment; I think we're still in an early stage, with many open questions. I guess a simple README would do the job in first approach.

Oh indeed that's really clever!
Should we split this PR by keeping only the database part, or do you think you can update it to support this new data structure?

I will take some note and make a small file with the essential guidelines

I did not want to change the screens in the first place, but I had too, because of the impact of the change of database.
If you give me a couple of days I think I can do something nice with ContinuousScanPage, in this PR. That will give us food for thought, and perhaps ideas of templates / for your guidelines.

I understand, that would be great. Once we have this page working we can transition the rest of the app to use the same principle and the document it (or probably the other way around).
I will wait for your update and we can discuss it then, let me know if you need any help.

…el without Widgets Impacted files: * `alternative_continuous_scan_page.dart`: refactoring mainly due to changes in `ContinuousScanModel` * `continuous_scan_model.dart`: now we don't deal with `Widget`s, only metadata, and we use a NotifyListeners method * `continuous_scan_page.dart`: refactoring mainly due to changes in `ContinuousScanModel` * `contribution_page.dart`: unrelated refactoring about the use of `UserPreferences` * `local_database.dart`: added a temporary "dummy"`NotifyListeners` method, for the sake of demonstration - to be removed * `smooth_product_carousel.dart`: now we build the `Widget`s from metadata, and we control here the carousel

monsieurtanuki · 2021-01-02T13:32:07Z

@PrimaelQuemerais Working again on the subject, I think the things are clearer now.

Tell me what you think of ContinousScanModel: the idea was to remove everything related to Widgets and CarouselController, and to leave it managing product metadata only. The model is more simple. The StatefulWidgets are more simple too.

For the moment the notifyListeners method is a dummy method created in LocalDatabase. The solution would be either to create a dedicated ChangeNotifier, something like ScanNotifier, or to store the scan list data in the database with the appropriate notifications.

PrimaelQuemerais · 2021-01-05T17:42:18Z

Sorry for the delay, I will review this after my exams this week.

monsieurtanuki · 2021-01-07T13:44:34Z

@PrimaelQuemerais Actually I have things to fix, and yes, I'll probably be using providers. The fact is that the keywords query currently fails (as already mentioned - double/null), and this possibility was not taken into account in the page.
Give me one day or two to clean the page.

New file: * `constant_icons.dart`: place to put icons that are different on iOS and Android (I know, the name of the class stinks) Impacted files: * `pubspec.yaml`: added `cupertino_icons`, at least for the back icon * `product_query_model.dart`: added `enum LoadingStatus` for a load step by step * `product_query_page.dart`: refactoring - we refresh the display according to the model's loading status and results, including errors

monsieurtanuki · 2021-01-07T21:15:42Z

@PrimaelQuemerais Now the part about ProductQueryModel looks good to me. I hope you'll find it good too ;)

M123-dev · 2021-01-07T23:21:45Z

packages/smooth_app/lib/database/local_database.dart

+    }
+    if (queryResult.length > 1) {
+      // very very unlikely to happen
+      throw Exception('Several products with the same barcode $barcode');


Before the release, we should still include a possibility to choose the desired product

The barcode is set as a Primary Key so queryResult.length will always be <= 1 😉

Of course it will always be 0 or 1. Unless something very strange happens, that I prefer to catch here. It's safer and it's an explicit way to tell the future contributor: "hey, this is 0 or 1 product".

If so, an exception is perfectly fine

Corrections for display exceptions

PrimaelQuemerais · 2021-01-08T00:22:41Z

I've started to look at it. I've made the necessary changes for the code to work with the new version of the openfoodfacts-dart plugin.
I need a little bit more time to look at everything, will let you know 🙂

monsieurtanuki · 2021-01-08T08:47:42Z

@PrimaelQuemerais I wouldn't have tried to put the new openfoodfacts: ^0.3.14+1 in the same move. But I'm not a git expert.

PrimaelQuemerais · 2021-01-08T09:13:15Z

@monsieurtanuki I somehow did it in the wrong order but the Master was also updated to use v0.3.14+1 (I added it to #65 which updates the dependencies).
But true it should've been done by merging Master into this branch, saw you did that thanks.

I'm facing quite a few display issues, I'll check all the database related code today so we can merge and then fix these issues.

monsieurtanuki · 2021-01-08T09:22:00Z

@PrimaelQuemerais I've just added a minor merge fix. I'm done.

PrimaelQuemerais · 2021-01-08T12:10:42Z

@monsieurtanuki I went through everything, I really like the implementation of the database it is really neat.
Regarding the state management context.watch<LocalDatabase> is working nicely, if I'm not mistaking we could set the ContinuousScanPage back to a StatelessWidget or am I omitting something?

monsieurtanuki · 2021-01-08T13:09:37Z

@PrimaelQuemerais I'm glad you liked the code!

Regarding ContinuousScanPage being a StatelessWidget, maybe you're right, try that. That would involve putting _continuousScanModel = ContinuousScanModel(contributionMode: widget.initializeWithContributionMode) in the constructor, but I believe you're rather supposed to use const fields; with StatefulWidget there's more flexibility.
But I think there's a problem with StatelessWidget: the whole widget would be rebuilt (triggered by the provider notification) and you would then create a new model from scratch. With the initState solution, you create the model just once and reuse it in the next build(context) calls.
LocalDatabase.dummyNotifyListeners() was just an initial suggestion, a dedicated ScanNotifier would make more sense.

That being said, if you think this PR is an improvement, I would suggest you to approve it and to merge it quickly, as it impacts many files and I would like to avoid the same merge conflicts again and again.
And there is probably refactoring needed regarding the providers, at least for coherence reasons, but that could be a new issue / a new PR.

PrimaelQuemerais · 2021-01-08T15:15:01Z

@monsieurtanuki I think you're right we would not gain anything from making the widget stateless. It is now merged into master

stephanegigandet · 2021-01-08T15:37:06Z

Thank you very much for all the great progress @monsieurtanuki and @PrimaelQuemerais !

monsieurtanuki · 2021-01-08T17:00:12Z

@stephanegigandet You're welcome! Currently working on the match / ranking / color features.

monsieurtanuki requested a review from PrimaelQuemerais January 1, 2021 20:12

PrimaelQuemerais reviewed Jan 1, 2021

View reviewed changes

PrimaelQuemerais linked an issue Jan 2, 2021 that may be closed by this pull request

Refactor the use of database #55

Closed

monsieurtanuki requested a review from PrimaelQuemerais January 2, 2021 13:26

M123-dev mentioned this pull request Jan 4, 2021

Darkmode #61

Merged

monsieurtanuki added 2 commits January 7, 2021 21:06

Merge branch 'master' into feature/#55

a8d8b47

M123-dev reviewed Jan 7, 2021

View reviewed changes

Changes to match v0.3.14+1 of openfoodfacts-dart

b8c6f42

Corrections for display exceptions

Merge branch 'master' into feature/#55

de8de42

Minor merge fix.

dd35d5c

PrimaelQuemerais merged commit ff4219a into openfoodfacts:master Jan 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/#55 - SQLite local database, and refactoring with StatefulWidget's #62

feature/#55 - SQLite local database, and refactoring with StatefulWidget's #62

monsieurtanuki commented Jan 1, 2021

PrimaelQuemerais left a comment •

edited

Loading

PrimaelQuemerais Jan 1, 2021 •

edited

Loading

monsieurtanuki Jan 1, 2021

PrimaelQuemerais Jan 1, 2021

monsieurtanuki Jan 1, 2021

PrimaelQuemerais Jan 1, 2021

monsieurtanuki Jan 1, 2021

PrimaelQuemerais Jan 1, 2021

monsieurtanuki commented Jan 2, 2021

PrimaelQuemerais commented Jan 5, 2021

monsieurtanuki commented Jan 7, 2021

monsieurtanuki commented Jan 7, 2021

M123-dev Jan 7, 2021

PrimaelQuemerais Jan 8, 2021

monsieurtanuki Jan 8, 2021

M123-dev Jan 8, 2021

PrimaelQuemerais commented Jan 8, 2021

monsieurtanuki commented Jan 8, 2021

PrimaelQuemerais commented Jan 8, 2021

monsieurtanuki commented Jan 8, 2021

PrimaelQuemerais commented Jan 8, 2021 •

edited

Loading

monsieurtanuki commented Jan 8, 2021

PrimaelQuemerais commented Jan 8, 2021

stephanegigandet commented Jan 8, 2021

monsieurtanuki commented Jan 8, 2021


		enum ScannedProductState { FOUND, NOT_FOUND, LOADING }

		class ContinuousScanModel extends ChangeNotifier {

feature/#55 - SQLite local database, and refactoring with StatefulWidget's #62

feature/#55 - SQLite local database, and refactoring with StatefulWidget's #62

Conversation

monsieurtanuki commented Jan 1, 2021

PrimaelQuemerais left a comment • edited Loading

Choose a reason for hiding this comment

PrimaelQuemerais Jan 1, 2021 • edited Loading

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 commented Jan 2, 2021

PrimaelQuemerais commented Jan 5, 2021

monsieurtanuki commented Jan 7, 2021

monsieurtanuki commented Jan 7, 2021

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

PrimaelQuemerais commented Jan 8, 2021

monsieurtanuki commented Jan 8, 2021

PrimaelQuemerais commented Jan 8, 2021

monsieurtanuki commented Jan 8, 2021

PrimaelQuemerais commented Jan 8, 2021 • edited Loading

monsieurtanuki commented Jan 8, 2021

PrimaelQuemerais commented Jan 8, 2021

stephanegigandet commented Jan 8, 2021

monsieurtanuki commented Jan 8, 2021

PrimaelQuemerais left a comment •

edited

Loading

PrimaelQuemerais Jan 1, 2021 •

edited

Loading

PrimaelQuemerais commented Jan 8, 2021 •

edited

Loading