-
-
Notifications
You must be signed in to change notification settings - Fork 279
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
feature/#55 - SQLite local database, and refactoring with StatefulWidget's #62
feature/#55 - SQLite local database, and refactoring with StatefulWidget's #62
Conversation
…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`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
enum ScannedProductState { FOUND, NOT_FOUND, LOADING } | ||
|
||
class ContinuousScanModel extends ChangeNotifier { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your point about the ChangeNotifier
vs. StatefulWidget
.
That being said, the code before I refactored it was really painful to read, with Consumer
s here and there.
My idea is to have most of the relevant data in the Provider
s 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really see how that will reduce the use of Consumers though.
That's the beauty of Provider
s: 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@PrimaelQuemerais Working again on the subject, I think the things are clearer now. Tell me what you think of For the moment the |
Sorry for the delay, I will review this after my exams this week. |
@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 - |
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
@PrimaelQuemerais Now the part about |
} | ||
if (queryResult.length > 1) { | ||
// very very unlikely to happen | ||
throw Exception('Several products with the same barcode $barcode'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before the release, we should still include a possibility to choose the desired product
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The barcode is set as a Primary Key so queryResult.length will always be <= 1 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, an exception is perfectly fine
Corrections for display exceptions
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. |
@PrimaelQuemerais I wouldn't have tried to put the new |
@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). 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. |
@PrimaelQuemerais I've just added a minor merge fix. I'm done. |
@monsieurtanuki I went through everything, I really like the implementation of the database it is really neat. |
@PrimaelQuemerais I'm glad you liked the code! Regarding 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. |
@monsieurtanuki I think you're right we would not gain anything from making the widget stateless. It is now merged into master |
Thank you very much for all the great progress @monsieurtanuki and @PrimaelQuemerais ! |
@stephanegigandet You're welcome! Currently working on the match / ranking / color features. |
New files:
barcode_product_query.dart
: API query / product by barcode (used to be infull_products_database.dart
)local_database.dart
: local SQLite database with one table (product
)Deleted files:
choose_page_model.dart
: moved the code toStatefulWidget
ChoosePage
full_product_database.dart
: database code was deprecated; API fields is now inProductQuery
Impacted files:
alternative_continuous_scan_page.dart
: now aStatefulWidget
; refactored withContinuousScanPage
build.gradle
: unrelated - lowered the AndroidminSdkVersion
from24
to19
, in order to run the app on my old smartphone ;)choose_page.dart
: refactoring as changed toStatefulWidget
continuous_scan_model.dart
: not aChangeNotifier
anymore - should be improved when we use product listcontinuous_scan_page.dart
: now aStatefulWidget
group_product_query.dart
: minor refactoring due to the changes inproduct_query.dart
keywords_product_query.dart
: minor refactoring due to the changes inproduct_query.dart
main.dart
: addedLocalDatabase
to the providers; simplified the code; removed mentions toSharedPreferences
product_query.dart
: refactored a bit; moved here some fields fromfull_product_database.dart
product_query_model.dart
: now usingLocalDatabase
product_query_page.dart
: now usingLocalDatabase
; refactoringpubspec.yaml
: replacedsembast with
sqflite`