feat: Better support error when saving the picture locally #4305

g123k · 2023-07-14T11:33:52Z

Hi everyone,

If something fails in the photo-taking process, there is no error catching.
I will now catch the error and also provide a timeout, where sometimes the Isolate has weird behaviors.
We also have a dedicated method to send custom exceptions to Sentry to better track this kind of issue.

Related issue: #4304

codecov-commenter · 2023-07-14T11:41:38Z

Codecov Report

Merging #4305 (8ed7a1b) into develop (1cd22a6) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #4305      +/-   ##
===========================================
- Coverage    10.80%   10.79%   -0.01%     
===========================================
  Files          287      287              
  Lines        14202    14208       +6     
===========================================
  Hits          1534     1534              
- Misses       12668    12674       +6

Impacted Files	Coverage Δ
...kages/smooth_app/lib/helpers/analytics_helper.dart	`0.00% <0.00%> (ø)`
packages/smooth_app/lib/pages/crop_page.dart	`0.00% <0.00%> (ø)`

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

monsieurtanuki

@g123k Interesting bug! Are you able to reproduce it?
I have no idea if an isolate is more likely to fail than a standard async. Perhaps more memory intensive?

What about

trying to use an isolate to save the picture (cf. the previous code and your PR)
if it failed, trying to use a "normal async" to save the picture: as a fallback, the picture is going to be saved but with a little screen freeze - which is more acceptable for the user than an enigmatic dialog
if it failed too, display an enigmatic dialog (cf. your PR)

Doing so, we'll deliver a better service to the user, while tracking and being able to understand how isolates (or async) may fail regarding saving a picture.

What do you think of that?

g123k · 2023-07-14T22:01:15Z

@g123k Interesting bug! Are you able to reproduce it? I have no idea if an isolate is more likely to fail than a standard async. Perhaps more memory intensive?

What about

trying to use an isolate to save the picture (cf. the previous code and your PR)

if it failed, trying to use a "normal async" to save the picture: as a fallback, the picture is going to be saved but with a little screen freeze - which is more acceptable for the user than an enigmatic dialog

if it failed too, display an enigmatic dialog (cf. your PR)

Doing so, we'll deliver a better service to the user, while tracking and being able to understand how isolates (or async) may fail regarding saving a picture.

What do you think of that?

My guess on this is sometimes Isolate doesn't start.
I don't really know why and the goal of this PR is just to understand if that's the root cause, thanks to the reporting to Sentry.

I'm not sure if it's worth the case to implement a better fix for now, as I can't reproduce it.
But clearly, if that's related to the Isolate, your solution should be implemented.

monsieurtanuki · 2023-07-15T05:57:29Z

My guess on this is sometimes Isolate doesn't start.

Makes sense.

I don't really know why and the goal of this PR is just to understand if that's the root cause, thanks to the reporting to Sentry.

I have nothing against this idea, but I don't know how clever we are to detect it. Let me rephrase: in your PR you added a call to Sentry, but Sentry is flooded by tons of error messages.

=> Could you share a Sentry link (or set up a Sentry alarm if that exists) about all the time this specific error is sent to Sentry?

I'm not sure if it's worth the case to implement a better fix for now, as I can't reproduce it.
But clearly, if that's related to the Isolate, your solution should be implemented.

I'll implement "my" solution in another PR - I've just assigned myself to the issue and made sure that when this PR is merged the issue won't be closed automatically.

g123k · 2023-07-15T08:26:35Z

FYI I've lost my access to Sentry. Once I have it back, it will share the link

g123k · 2023-07-17T07:58:47Z

For Sentry; you just have to enter the method's name to filter all exceptions: https://openfoodfacts.sentry.io/issues/?project=5376745&query=is%3Aunresolved+_getCroppedImageFile&referrer=issue-list&statsPeriod=1h

g123k · 2023-07-17T08:04:00Z

I will change a bit the implementation to show a dialog with an error message

g123k · 2023-07-17T08:30:51Z

That should be better now, with a dedicated message: ErrorDialog.webm 🎬.
I've also created a SmoothSimpleErrorAlertDialog that we can reuse in the app.

…version…", we will now send the error to the server

…the image processing

monsieurtanuki

Hi @g123k!
Please have a look at my comments.

packages/smooth_app/lib/pages/crop_page.dart

monsieurtanuki · 2023-07-17T08:47:14Z

packages/smooth_app/lib/pages/crop_page.dart

+  void _showErrorDialog() {
+    final AppLocalizations appLocalizations = AppLocalizations.of(context);
+
+    showDialog<void>(


Mmm that's meaningless here, as we're not statement after.
Why would it be necessary?

I'm not sure if it's "necessary", but it would be cleaner and more consistent with the rest of the showDialog calls in the app.

I've added a Future to the method, which should be better

github-actions · 2023-07-17T09:01:35Z

You can test this PR on: https://app.waldo.com/applications/app-19d476740ba1bb36/sessions

g123k added 🖼️ Photos - Cropping error handling labels Jul 14, 2023

g123k requested a review from a team as a code owner July 14, 2023 11:33

g123k self-assigned this Jul 14, 2023

github-actions bot added 📈 Analytics We use Sentry and Matomo, with an opt-in system 🌐 l10n and removed 🖼️ Photos - Cropping labels Jul 14, 2023

monsieurtanuki reviewed Jul 14, 2023

View reviewed changes

monsieurtanuki mentioned this pull request Jul 15, 2023

Seldom I can't take a picture #4304

Closed

monsieurtanuki mentioned this pull request Jul 16, 2023

fix: 4304 - save images as async instead of isolate (fallback) #4323

Merged

g123k marked this pull request as draft July 17, 2023 08:03

g123k marked this pull request as ready for review July 17, 2023 08:30

g123k requested a review from monsieurtanuki July 17, 2023 08:31

openfoodfacts deleted a comment from github-actions bot Jul 17, 2023

g123k added 3 commits July 17, 2023 10:48

In the case where the user is stuck with the message "Saving a local …

35fddbe

…version…", we will now send the error to the server

Instead a cryptic message, explain to the user there's an issue with …

8f7a0b0

…the image processing

Rebase the code to let the test pass

8943f15

g123k force-pushed the error_saving_local_image branch from 84776aa to 8943f15 Compare July 17, 2023 08:48

monsieurtanuki reviewed Jul 17, 2023

View reviewed changes

Revert the fact that _getCroppedImageFile can be nullable

fdc435c

monsieurtanuki approved these changes Jul 17, 2023

View reviewed changes

Add a Future return to _showErrorDialog

6444246

g123k merged commit 5a72a46 into openfoodfacts:develop Jul 17, 2023
7 checks passed

g123k deleted the error_saving_local_image branch July 17, 2023 11:17

github-actions bot mentioned this pull request Jul 17, 2023

chore(develop): release 4.9.0 #4257

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Better support error when saving the picture locally #4305

feat: Better support error when saving the picture locally #4305

g123k commented Jul 14, 2023 •

edited by monsieurtanuki

Loading

codecov-commenter commented Jul 14, 2023

monsieurtanuki left a comment

g123k commented Jul 14, 2023

monsieurtanuki commented Jul 15, 2023

g123k commented Jul 15, 2023

g123k commented Jul 17, 2023

g123k commented Jul 17, 2023

g123k commented Jul 17, 2023

monsieurtanuki left a comment

monsieurtanuki Jul 17, 2023

g123k Jul 17, 2023

monsieurtanuki Jul 17, 2023

g123k Jul 17, 2023

github-actions bot commented Jul 17, 2023

feat: Better support error when saving the picture locally #4305

feat: Better support error when saving the picture locally #4305

Conversation

g123k commented Jul 14, 2023 • edited by monsieurtanuki Loading

codecov-commenter commented Jul 14, 2023

Codecov Report

monsieurtanuki left a comment

Choose a reason for hiding this comment

g123k commented Jul 14, 2023

monsieurtanuki commented Jul 15, 2023

g123k commented Jul 15, 2023

g123k commented Jul 17, 2023

g123k commented Jul 17, 2023

g123k commented Jul 17, 2023

monsieurtanuki left a comment

Choose a reason for hiding this comment

monsieurtanuki Jul 17, 2023

Choose a reason for hiding this comment

g123k Jul 17, 2023

Choose a reason for hiding this comment

monsieurtanuki Jul 17, 2023

Choose a reason for hiding this comment

g123k Jul 17, 2023

Choose a reason for hiding this comment

github-actions bot commented Jul 17, 2023

g123k commented Jul 14, 2023 •

edited by monsieurtanuki

Loading