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: 4941 - refresh products when switching app language #5016

Merged
merged 12 commits into from
Feb 24, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
import 'package:flutter/painting.dart';
import 'package:flutter_gen/gen_l10n/app_localizations.dart';
import 'package:openfoodfacts/openfoodfacts.dart';
import 'package:smooth_app/background/background_task.dart';
import 'package:smooth_app/background/operation_type.dart';
import 'package:smooth_app/database/dao_product.dart';
import 'package:smooth_app/database/local_database.dart';
import 'package:smooth_app/query/product_query.dart';

/// Background task about downloading products to translate.
class BackgroundTaskLanguageRefresh extends BackgroundTask {
BackgroundTaskLanguageRefresh._({
required super.processName,
required super.uniqueId,
required super.stamp,
required this.excludeBarcodes,
});

BackgroundTaskLanguageRefresh.fromJson(Map<String, dynamic> json)
: excludeBarcodes = _getStringList(json, _jsonTagExcludeBarcodes),
super.fromJson(json);

static List<String> _getStringList(
final Map<String, dynamic> json, final String tag) {
final List<dynamic> dynamicList =
json[_jsonTagExcludeBarcodes] as List<dynamic>;
final List<String> result = <String>[];
for (final dynamic item in dynamicList) {
result.add(item.toString());
}
return result;
}

final List<String> excludeBarcodes;

static const String _jsonTagExcludeBarcodes = 'excludeBarcodes';

@override
Map<String, dynamic> toJson() {
final Map<String, dynamic> result = super.toJson();
result[_jsonTagExcludeBarcodes] = excludeBarcodes;
return result;
}

static const OperationType _operationType = OperationType.languageRefresh;

static Future<void> addTask(
final LocalDatabase localDatabase, {
final List<String> excludeBarcodes = const <String>[],
}) async {
final String uniqueId = await _operationType.getNewKey(
localDatabase,
);
final BackgroundTask task = _getNewTask(
uniqueId,
excludeBarcodes,
);
await task.addToManager(localDatabase);
}

@override
(String, AlignmentGeometry)? getFloatingMessage(
final AppLocalizations appLocalizations) =>
null;

static BackgroundTask _getNewTask(
final String uniqueId,
final List<String> excludeBarcodes,
) =>
BackgroundTaskLanguageRefresh._(
processName: _operationType.processName,
uniqueId: uniqueId,
stamp: ';languageRefresh',
excludeBarcodes: excludeBarcodes,
);

@override
Future<void> preExecute(final LocalDatabase localDatabase) async {}

@override
bool get hasImmediateNextTask => true;
Copy link
Member

Choose a reason for hiding this comment

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

It's not always the case. I'm again doing a mobile review so I can't double-check the implications but I guess it's not causing any problems, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bool get hasImmediateNextTask => true;

@M123-dev When we page our background tasks, task A will deal with only say 10 products and at the end will say "I know there are more products to deal with and I create task B". In that case we need the background manager to immediately restart, to deal with task B.
In case we have no more products to deal with, no additional task will be created and the manager will see an empty task queue, and do nothing.

As opposed to standard one-shot tasks (e.g. product detail change), that are not somehow chained to other tasks to be run immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm again doing a mobile review so I can't double-check the implications

Many thanks @M123-dev, that's not the easiest PR to review on mobile ;)


/// Number of products to download each time.
static const int _pageSize = 20;

@override
Future<void> execute(final LocalDatabase localDatabase) async {
final DaoProduct daoProduct = DaoProduct(localDatabase);
final OpenFoodFactsLanguage language = ProductQuery.getLanguage();
final List<String> barcodes = await daoProduct.getTopProductsToTranslate(
language,
limit: _pageSize,
excludeBarcodes: excludeBarcodes,
);
if (barcodes.isEmpty) {
return;
}
final SearchResult searchResult = await OpenFoodAPIClient.searchProducts(
getUser(),
ProductSearchQueryConfiguration(
fields: ProductQuery.fields,
parametersList: <Parameter>[
const PageSize(size: _pageSize),
const PageNumber(page: 1),
BarcodeParameter.list(barcodes),
],
language: language,
country: ProductQuery.getCountry(),
version: ProductQuery.productQueryVersion,
),
uriHelper: uriProductHelper,
);
if (searchResult.products == null || searchResult.count == null) {
throw Exception('Cannot refresh language');
}

// save into database and refresh all visible products.
await daoProduct.putAll(searchResult.products!, language);
localDatabase.upToDate.setLatestDownloadedProducts(searchResult.products!);

// Next page
final List<String> newExcludeBarcodes = <String>[];
// we keep the old "excluded" barcodes,...
newExcludeBarcodes.addAll(excludeBarcodes);
// ...add the new barcodes...
newExcludeBarcodes.addAll(barcodes);
// ...and remove barcodes we actually found on the server.
for (final Product product in searchResult.products!) {
newExcludeBarcodes.remove(product.barcode);
Comment on lines +127 to +129
Copy link
Member

Choose a reason for hiding this comment

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

Say we try to refresh a product that was removed in the time before the refresh, then we'll have a lopped until the Cannot refresh language exception

Let's remove the ones we queried, not the ones we received

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's remove the ones we queried, not the ones we received

First we add all the barcodes we expected, and then remove all the barcodes we received. In the end we exclude the barcodes we expected but haven't received.
Here we're dealing with an exclusion barcode list: we need to tell the next task "I have already tried this barcode but received nothing".

    // ...add the new barcodes...
    newExcludeBarcodes.addAll(barcodes);
    // ...and remove barcodes we actually found on the server.
    for (final Product product in searchResult.products!) {
      newExcludeBarcodes.remove(product.barcode);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@M123-dev Just to clarify a bit:

  1. we run a query in the local database - "which products are not yet in the expected language?"
  2. we maintain a list of "excluded" barcodes - barcodes we already tried to download but were not returned by the server
  3. the products returned by the server won't get listed again by the (1) query, as they are now flagged with the expected language
  4. and then we go back to (1) query in the next background task, looking for products not in the expected language and not queried yet.

Copy link
Member

Choose a reason for hiding this comment

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

Oh okay, yeah makes more sense reading it now

}
await addTask(
localDatabase,
excludeBarcodes: newExcludeBarcodes,
);
}
}
12 changes: 10 additions & 2 deletions packages/smooth_app/lib/background/background_task_manager.dart
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class BackgroundTaskManager {
);
await DaoStringList(localDatabase).add(DaoStringList.keyTasks, taskId);
await task.preExecute(localDatabase);
run(); // no await
run();
}

/// Finishes a task cleanly.
Expand Down Expand Up @@ -80,11 +80,13 @@ class BackgroundTaskManager {
final String processName = BackgroundTask.getProcessName(map);
for (final OperationType operationType in OperationType.values) {
if (processName == operationType.processName) {
_debugPrint('found: $processName, $map');
return operationType.fromJson(map);
}
}
} catch (e) {
// unexpected
_debugPrint('_get exception: $e');
}
return null;
}
Expand Down Expand Up @@ -140,14 +142,20 @@ class BackgroundTaskManager {

bool _running = false;

/// Runs all the pending tasks, and then smoothly ends, without awaiting.
void run() {
// no await
_runAsync();
}

/// Runs all the pending tasks, and then smoothly ends.
///
/// If a task fails, we continue with the other tasks: and we'll retry the
/// failed tasks later.
/// If a task fails and another task with the same stamp comes after,
/// we can remove the failed task from the list: it would have been
/// overwritten anyway.
Future<void> run() async {
Future<void> _runAsync() async {
final int? now = _canStartNow();
if (now == null) {
return;
Expand Down
6 changes: 6 additions & 0 deletions packages/smooth_app/lib/background/operation_type.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'package:smooth_app/background/background_task_download_products.dart';
import 'package:smooth_app/background/background_task_full_refresh.dart';
import 'package:smooth_app/background/background_task_hunger_games.dart';
import 'package:smooth_app/background/background_task_image.dart';
import 'package:smooth_app/background/background_task_language_refresh.dart';
import 'package:smooth_app/background/background_task_offline.dart';
import 'package:smooth_app/background/background_task_progressing.dart';
import 'package:smooth_app/background/background_task_refresh_later.dart';
Expand Down Expand Up @@ -33,6 +34,7 @@ enum OperationType {
offlineBarcodes('B', 'OFFLINE_BARCODES'),
offlineProducts('P', 'OFFLINE_PRODUCTS'),
fullRefresh('F', 'FULL_REFRESH'),
languageRefresh('L', 'LANGUAGE_REFRESH'),
details('D', 'PRODUCT_EDIT');

const OperationType(this.header, this.processName);
Expand Down Expand Up @@ -85,6 +87,8 @@ enum OperationType {
return BackgroundTaskDownloadProducts.fromJson(map);
case fullRefresh:
return BackgroundTaskFullRefresh.fromJson(map);
case languageRefresh:
return BackgroundTaskLanguageRefresh.fromJson(map);
}
}

Expand Down Expand Up @@ -113,6 +117,8 @@ enum OperationType {
return 'Downloading products';
case OperationType.fullRefresh:
return 'Refreshing the full local database';
case OperationType.languageRefresh:
return 'Refreshing the local database to a new language';
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/smooth_app/lib/data_models/up_to_date_mixin.dart
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ mixin UpToDateMixin<T extends StatefulWidget> on State<T> {
/// To be used in the `build` method, after a call to
/// `context.watch<LocalDatabase>()`.
void refreshUpToDate() {
BackgroundTaskManager.getInstance(_localDatabase).run(); // no await
BackgroundTaskManager.getInstance(_localDatabase).run();
_refreshUpToDate();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,8 @@ class UpToDateProductProvider {

/// Typical use-case: a product list page is refreshed through a pull-gesture.
void setLatestDownloadedProducts(
final Iterable<Product> products, {
final bool notify = true,
}) {
final Iterable<Product> products,
) {
if (_interest.isEmpty) {
return;
}
Expand All @@ -88,7 +87,7 @@ class UpToDateProductProvider {
setLatestDownloadedProduct(product, notify: false);
}
}
if (notify && atLeastOne) {
if (atLeastOne) {
localDatabase.notifyListeners();
}
}
Expand Down
47 changes: 47 additions & 0 deletions packages/smooth_app/lib/database/dao_product.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import 'package:openfoodfacts/openfoodfacts.dart';
import 'package:smooth_app/database/abstract_sql_dao.dart';
import 'package:smooth_app/database/bulk_deletable.dart';
import 'package:smooth_app/database/bulk_manager.dart';
import 'package:smooth_app/database/dao_product_last_access.dart';
import 'package:smooth_app/database/local_database.dart';
import 'package:sqflite/sqflite.dart';

Expand Down Expand Up @@ -255,4 +256,50 @@ class DaoProduct extends AbstractSqlDao implements BulkDeletable {
// We return the number of rows deleted ie the number of products deleted
return localDatabase.database.delete(_TABLE_PRODUCT);
}

/// Returns the most recently locally accessed products with wrong language.
///
/// Typical use-case: when the user changes the app language, downloading
/// incrementally all products with a different (or null) download language.
/// We need [excludeBarcodes] because in some rare cases products may not be
/// found anymore on the server - it happened to me with obviously fake test
/// products being probably wiped out.
Future<List<String>> getTopProductsToTranslate(
final OpenFoodFactsLanguage language, {
required final int limit,
required final List<String> excludeBarcodes,
}) async {
final List<Map<String, dynamic>> queryResults =
await localDatabase.database.rawQuery(
'select p.$_TABLE_PRODUCT_COLUMN_BARCODE '
'from'
' $_TABLE_PRODUCT p'
' left outer join ${DaoProductLastAccess.TABLE} a'
' on p.$_TABLE_PRODUCT_COLUMN_BARCODE = a.${DaoProductLastAccess.COLUMN_BARCODE} '
'where'
' p.$_TABLE_PRODUCT_COLUMN_LANGUAGE is null'
' or p.$_TABLE_PRODUCT_COLUMN_LANGUAGE != ? '
'order by a.${DaoProductLastAccess.COLUMN_LAST_ACCESS} desc nulls last '
'limit ?',
<Object>[
language.offTag,
limit + excludeBarcodes.length,
],
);

final List<String> result = <String>[];

for (final Map<String, dynamic> row in queryResults) {
final String barcode = row[_TABLE_PRODUCT_COLUMN_BARCODE] as String;
if (excludeBarcodes.contains(barcode)) {
continue;
}
result.add(barcode);
if (result.length == limit) {
break;
}
}

return result;
}
}
2 changes: 1 addition & 1 deletion packages/smooth_app/lib/database/local_database.dart
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class LocalDatabase extends ChangeNotifier {

@override
void notifyListeners() {
BackgroundTaskManager.getInstance(this).run(); // no await
BackgroundTaskManager.getInstance(this).run();
super.notifyListeners();
}

Expand Down
2 changes: 1 addition & 1 deletion packages/smooth_app/lib/pages/offline_tasks_page.dart
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class _OfflineTaskState extends State<OfflineTaskPage> {
),
actions: <Widget>[
IconButton(
onPressed: () => // no await
onPressed: () =>
BackgroundTaskManager.getInstance(localDatabase).run(),
icon: const Icon(Icons.refresh),
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ import 'package:flutter/material.dart';
import 'package:flutter_gen/gen_l10n/app_localizations.dart';
import 'package:openfoodfacts/openfoodfacts.dart';
import 'package:provider/provider.dart';
import 'package:smooth_app/background/background_task_language_refresh.dart';
import 'package:smooth_app/data_models/preferences/user_preferences.dart';
import 'package:smooth_app/data_models/product_preferences.dart';
import 'package:smooth_app/database/local_database.dart';
import 'package:smooth_app/generic_lib/design_constants.dart';
import 'package:smooth_app/generic_lib/widgets/language_selector.dart';
import 'package:smooth_app/pages/preferences/user_preferences_item.dart';
Expand All @@ -24,19 +26,6 @@ class UserPreferencesLanguageSelector extends StatelessWidget {
);
}

Future<void> _changeAppLanguage(
BuildContext context,
UserPreferences userPreferences, {
required OpenFoodFactsLanguage language,
}) async {
ProductQuery.setLanguage(
context,
userPreferences,
languageCode: language.code,
);
await context.read<ProductPreferences>().refresh();
}

@override
Widget build(BuildContext context) {
final AppLocalizations appLocalizations = AppLocalizations.of(context);
Expand All @@ -56,12 +45,19 @@ class UserPreferencesLanguageSelector extends StatelessWidget {
if (language == null) {
return;
}

_changeAppLanguage(
ProductQuery.setLanguage(
context,
userPreferences,
language: language,
languageCode: language.code,
);
final ProductPreferences productPreferences =
context.read<ProductPreferences>();
await BackgroundTaskLanguageRefresh.addTask(
context.read<LocalDatabase>(),
);
// TODO(monsieurtanuki): make it a background task also?
// no await
productPreferences.refresh();
},
selectedLanguages: <OpenFoodFactsLanguage>[
ProductQuery.getLanguage(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,6 @@ class ProductRefresher {
if (result.product != null) {
await DaoProduct(localDatabase).put(result.product!, language);
localDatabase.upToDate.setLatestDownloadedProduct(result.product!);
localDatabase.notifyListeners();
return FetchedProduct.found(result.product!);
}
return const FetchedProduct.internetNotFound();
Expand Down Expand Up @@ -217,7 +216,6 @@ class ProductRefresher {
await DaoProduct(localDatabase).putAll(searchResult.products!, language);
localDatabase.upToDate
.setLatestDownloadedProducts(searchResult.products!);
localDatabase.notifyListeners();
return searchResult.products!.length;
} catch (e) {
Logs.e('Refresh from server error', ex: e);
Expand Down
Loading