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: 3263 - new BackgroundTaskManager that always works #3339

Merged
merged 4 commits into from
Nov 25, 2022

Conversation

monsieurtanuki
Copy link
Contributor

Deleted file:

  • background_task_helper.dart

New files:

  • background_task_manager.dart: Management of background tasks: single thread, block, restart, display.
  • dao_instant_string.dart: Where we store strings that need INSTANT access (= not lazy, no await).

Impacted fles:

  • abstract_background_task.dart: refactored
  • background_task_details.dart: refactored around the changes in AbstractBackgroundTask
  • background_task_image.dart: refactored around the changes in AbstractBackgroundTask
  • dao_string_list.dart: refactoring around now managing several lists; removed unnecessary await for a non-lazy dao
  • local_database.dart: added the new class DaoInstantString; relaunch the background task manager at every refresh
  • main.dart: minor refactoring
  • new_crop_page.dart: unrelated bug fix
  • offline_tasks_page.dart: refactored around the new BackgroundTaskManager
  • operation_type.dart: added helper methods
  • product_image_gallery_view.dart: minor refactoring
  • product_image_viewer.dart: unrelated bug fix - the product was not refreshed, and so wasn't the image even after a successful download
  • pubspec.lock: wtf
  • pubspec.yaml: removed flutter_task_manager
  • search_history_view.dart: minor refactoring now that we have several lists in DaoStringList
  • search_page.dart: minor refactoring now that we have several lists in DaoStringList
  • up_to_date_changes.dart: minor refactoring
  • up_to_date_product_provider.dart: minor refactoring

What

  • The Background Task Management has been refactored.
  • Given all the specificities in Smoothie's tasks (e.g. pretending data is already uploaded when it's not), a more generic task manager like the previous one was not appropriate.
  • Now we store a list of tasks (detail changes or image uploads).
  • The tasks are run frequently - each time the database is refreshed (localDatabase.notofyListeners())
  • When we run tasks, we run sequentially each task, and if one fails, we stop. That can be problematic.
  • We never run 2 tasks at the same time.
  • When the latest task was run more than one hour ago, we assume it failed (presumably because of the server) and can run it again
  • We can block the tasks - I don't know how relevant it can be in release but it's useful in debug, as shown in the screenshot.
  • Improvements could be around when to try again
    • the way it's coded now, the manager in offline will try again and again to run tasks (that will fail). We could also wait 5 minutes after each try.
    • maybe there would be specifically appropriates time when to try again

Screenshot

Capture d’écran 2022-11-21 à 19 10 07

Part of

Deleted file:
* `background_task_helper.dart`

New files:
* `background_task_manager.dart`: Management of background tasks: single thread, block, restart, display.
* `dao_instant_string.dart`: Where we store strings that need INSTANT access (= not lazy, no await).

Impacted fles:
* `abstract_background_task.dart`: refactored
* `background_task_details.dart`: refactored around the changes in `AbstractBackgroundTask`
* `background_task_image.dart`: refactored around the changes in `AbstractBackgroundTask`
* `dao_string_list.dart`: refactoring around now managing several lists; removed unnecessary `await` for a non-lazy dao
* `local_database.dart`: added the new class `DaoInstantString`; relaunch the background task manager at every refresh
* `main.dart`: minor refactoring
* `new_crop_page.dart`: unrelated bug fix
* `offline_tasks_page.dart`: refactored around the new `BackgroundTaskManager`
* `operation_type.dart`: added helper methods
* `product_image_gallery_view.dart`: minor refactoring
* `product_image_viewer.dart`: unrelated bug fix - the product was not refreshed, and so wasn't the image even after a successful download
* `pubspec.lock`: wtf
* `pubspec.yaml`: removed `flutter_task_manager`
* `search_history_view.dart`: minor refactoring now that we have several lists in `DaoStringList`
* `search_page.dart`: minor refactoring now that we have several lists in `DaoStringList`
* `up_to_date_changes.dart`: minor refactoring
* `up_to_date_product_provider.dart`: minor refactoring
@monsieurtanuki monsieurtanuki requested a review from a team as a code owner November 21, 2022 19:25
@github-actions github-actions bot added database dependencies 🥫 Product page 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… labels Nov 21, 2022
@monsieurtanuki monsieurtanuki changed the title feature: 3263 - new BackgroundTaskManager that always works feat: 3263 - new BackgroundTaskManager that always works Nov 21, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #3339 (0cce8be) into develop (a141bdf) will decrease coverage by 0.01%.
The diff coverage is 0.53%.

@@             Coverage Diff             @@
##           develop    #3339      +/-   ##
===========================================
- Coverage    10.49%   10.47%   -0.02%     
===========================================
  Files          253      254       +1     
  Lines        12313    12336      +23     
===========================================
  Hits          1292     1292              
- Misses       11021    11044      +23     
Impacted Files Coverage Δ
...h_app/lib/background/abstract_background_task.dart 0.00% <0.00%> (ø)
...th_app/lib/background/background_task_details.dart 0.00% <0.00%> (ø)
...ooth_app/lib/background/background_task_image.dart 0.00% <0.00%> (ø)
...th_app/lib/background/background_task_manager.dart 0.00% <0.00%> (ø)
...ges/smooth_app/lib/data_models/operation_type.dart 5.00% <0.00%> (-3.34%) ⬇️
...smooth_app/lib/data_models/up_to_date_changes.dart 0.00% <0.00%> (ø)
...p/lib/data_models/up_to_date_product_provider.dart 0.00% <0.00%> (ø)
...es/smooth_app/lib/database/dao_instant_string.dart 0.00% <0.00%> (ø)
...kages/smooth_app/lib/database/dao_string_list.dart 0.00% <0.00%> (ø)
...ckages/smooth_app/lib/database/local_database.dart 0.00% <0.00%> (ø)
... and 8 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@teolemon
Copy link
Member

@monsieurtanuki small conflict to solve

Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

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

Now we don't use @g123k's task_manager anymore, so there is no real background task at all, just background while the app is running?

@monsieurtanuki
Copy link
Contributor Author

Now we don't use @g123k's task_manager anymore, so there is no real background task at all, just background while the app is running?

To me in flutter (which is mono-threaded) there are sync methods, async methods, and isolates.
In this PR there are no isolates, just async methods run without awaiting, that perform tasks that are not strictly related to the displayed page. That does sound like "background tasks" to me, like a Java garbage collector that runs at "random" moments and do things not strictly related to the current display.

@M123-dev
Copy link
Member

To me in flutter (which is mono-threaded) there are sync methods, async methods, and isolates.
In this PR there are no isolates, just async methods run without awaiting, that perform tasks that are not strictly related to the displayed page. That does sound like "background tasks" to me, like a Java garbage collector that runs at "random" moments and do things not strictly related to the current display.

Thats why the name BackgroundTaskManager is ironic. You removed the background tasks. I have only played a very small part in it's creation but still I think it makes sense. We can't hide the progress of something if it just pauses when we close the app.

@monsieurtanuki
Copy link
Contributor Author

@M123-dev I think I understand what you mean: something called Services in Android. Is that what you were thinking about?

I don't know how to implement those services in flutter, but I think it would anyway be overkill here: we're talking about quick actions that are not CPU-consuming, and "my" BackgroundTaskManager is only 144 line long. It works, especially given our specific needs, and we can maintain it.

Again, I would still call my code "background" tasks, like I would say "background" image: it's there, behind the rest.
According to wikipedia: "A background process is a computer process that runs behind the scenes (i.e., in the background) and without user intervention."

If you find more appropriate wordings for my classes, feel free to share it with me. Spoiler alert: I will need help in new class wording in openfoodfacts/openfoodfacts-dart#617, probably tomorrow.

@AshAman999
Copy link
Member

Now we don't use @g123k's task_manager anymore, so there is no real background task at all, just background while the app is running?

Even with task manager we were just doing the work in app background, there wasn't isolates even then. Im terms of functionality I don't think that there will be any major difference if we do the sync with this code.

Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

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

Besides my above mentioned concerns the code looks good to me 👍🏼

@monsieurtanuki monsieurtanuki merged commit 5304614 into openfoodfacts:develop Nov 25, 2022
@monsieurtanuki
Copy link
Contributor Author

Thank you very much @M123-dev for the review!
We'll see in the long run if we need Services.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database dependencies 🥫 Product page 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion…
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants