-
-
Notifications
You must be signed in to change notification settings - Fork 313
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: #2785 - async access to dao product list #2788
feat: #2785 - async access to dao product list #2788
Conversation
Impacted files: * `all_user_product_list_page.dart`: split in a not-loaded / loaded version; computes each user list length with a `FutureBuilder * `continuous_scan_model.dart`: minor refactoring * `dao_product_list.dart`: switch from "box" to "lazyBox" in order to cleanly make all methods `async` * `new_product_page.dart`: now loading lists with `FutureBuilder` * `product_list_page.dart`: minor refactoring * `product_list_supplier.dart`: minor refactoring * `product_list_user_dialog_helper.dart`: now explicitly calling `notifyListeners` as `setState` won't be enough because of pre-loaded data * `query_product_list_supplier.dart`: minor refactoring
Impacted files: * `paged_product_query.dart`: replaced deprecated methods * `paged_to_be_completed_product_query.dart`: replaced deprecated methods * `product_list_import_export.dart`: replaced deprecated methods * `product_list_page.dart`: replaced deprecated methods * `pubspec.lock`: generated * `pubspec.yaml`: upgrade to `openfoodfacts: ^1.24.0`
final BuildContext context, | ||
final AsyncSnapshot<List<String>> snapshot, | ||
) { | ||
if (snapshot.data != null && snapshot.data!.isNotEmpty) { |
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 would invert the if so that we do the "heavy lifting" outside the if branch
final BuildContext context, | ||
final AsyncSnapshot<int> snapshot, | ||
) { | ||
if (snapshot.data != null) { |
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 would invert this if for readability
Thank you @VaiTon for your review! I would agree with you about the position of |
Impacted files:
all_user_product_list_page.dart
: split in a not-loaded / loaded version; computes each user list length with aFutureBuilder
continuous_scan_model.dart
: minor refactoringdao_product_list.dart
: switch from "box" to "lazyBox" in order to cleanly make all methodsasync
new_product_page.dart
: now loading lists withFutureBuilder
product_list_page.dart
: minor refactoringproduct_list_supplier.dart
: minor refactoringproduct_list_user_dialog_helper.dart
: now explicitly callingnotifyListeners
assetState
won't be enough because of pre-loaded dataquery_product_list_supplier.dart
: minor refactoringWhat
async
.sqflite
, which has a behavior similar tohive
's lazy boxes - very fast init, no pre-load and async access.sqflite
, we won't have to worry aboutasync
accesses - it's already done here. Less impact on less files, which is easier to review ;)sqflite
now. That wouldn't be a great idea because we should deal also with the "bigger primary key" feature, in order to migrate at the same time enhanced primary keys and hive-to-sqflite.Part of