-
-
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
Merged
PrimaelQuemerais
merged 7 commits into
openfoodfacts:master
from
monsieurtanuki:feature/#55
Jan 8, 2021
Merged
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
2dbb6b6
feature/#55 - SQLite local database, and refactoring with StatefulWid…
monsieurtanuki c5f5b93
feature/#55 - experimenting the NotifyListeners method in a clean mod…
monsieurtanuki ee64551
Clean separation between model and page around product query
monsieurtanuki a8d8b47
Merge branch 'master' into feature/#55
monsieurtanuki b8c6f42
Changes to match v0.3.14+1 of openfoodfacts-dart
de8de42
Merge branch 'master' into feature/#55
monsieurtanuki dd35d5c
Minor merge fix.
monsieurtanuki File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
101 changes: 0 additions & 101 deletions
101
packages/smooth_app/lib/data_models/choose_page_model.dart
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
27 changes: 27 additions & 0 deletions
27
packages/smooth_app/lib/database/barcode_product_query.dart
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import 'dart:async'; | ||
|
||
import 'package:openfoodfacts/openfoodfacts.dart'; | ||
import 'package:smooth_app/database/product_query.dart'; | ||
import 'package:openfoodfacts/utils/LanguageHelper.dart'; | ||
|
||
class BarcodeProductQuery { | ||
BarcodeProductQuery(this.barcode); | ||
|
||
final String barcode; | ||
|
||
Future<Product> getProduct() async { | ||
final ProductQueryConfiguration configuration = ProductQueryConfiguration( | ||
barcode, | ||
fields: ProductQuery.fields, | ||
language: OpenFoodFactsLanguage.ENGLISH, | ||
); | ||
|
||
final ProductResult result = | ||
await OpenFoodAPIClient.getProduct(configuration); | ||
|
||
if (result.status == 1) { | ||
return result.product; | ||
} | ||
return null; | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 themain.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 mixingChangeNotifiers
andStatefulWidgets
can become confusing. Perhaps we should determine a clear rule on whenProvider
orStatefulWidgets
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 ofConsumers
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.
That's the beauty of
Provider
s: if I start mybuild(BuildContext context)
with acontext.watch<LocalDatabase>()
, every time the notify listener method is called onLocalDatabase
, theWidget
is rebuilt. And I don't need anyConsumer
.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.