feat: Extract ingredients/packaging: loading / loaded / extracting states #5384

g123k · 2024-06-15T20:31:24Z

Hi everyone,

Extract ingredients and packaging share the same screen.
However, there are two issues:

We don't clearly understand what's happening with the progress bar
There is an empty container when we extract the data

I've made things clearer, that you can see in this video:

Untitled.mov

codecov-commenter · 2024-06-15T20:39:45Z

Codecov Report

Attention: Patch coverage is 0% with 78 lines in your changes missing coverage. Please review.

Project coverage is 7.32%. Comparing base (4d9c7fc) to head (d854301).
Report is 196 commits behind head on develop.

Files	Patch %	Lines
...es/smooth_app/lib/pages/product/edit_ocr_page.dart	0.00%	69 Missing ⚠️
..._app/lib/pages/product/ocr_ingredients_helper.dart	0.00%	5 Missing ⚠️
...th_app/lib/pages/product/ocr_packaging_helper.dart	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           develop   #5384      +/-   ##
==========================================
- Coverage     9.54%   7.32%   -2.23%     
==========================================
  Files          325     386      +61     
  Lines        16411   19847    +3436     
==========================================
- Hits          1567    1454     -113     
- Misses       14844   18393    +3549

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

monsieurtanuki

Hi @g123k!
LGTM, and this page did need some action!
Regarding the code, there are a couple of issues that bother me: changing translation tags, and not catching an exception.
The others are minor comments, feel free to ignore them.

monsieurtanuki · 2024-06-16T06:03:09Z

packages/smooth_app/lib/l10n/app_en.arb

@@ -1448,18 +1448,34 @@
    "completed_basic_details_btn_text": "Complete basic details",
    "not_implemented_snackbar_text": "Not implemented yet",
    "category_picker_page_appbar_text": "Categories",
-    "edit_ingredients_extrait_ingredients_btn_text": "Extract ingredients",
-    "@edit_ingredients_extrait_ingredients_btn_text": {
+    "edit_ingredients_extract_ingredients_btn_text": "Extract ingredients from the photo",


I'm really not convinced it's such a brillant idea to change translation tags, as I fear it wipes out all the previous translations. For a very limited added-value: fixing an invisible useless typo.

Ok, it's now merged :/
@teleomon Is-it somehow possible to move an old translation to a new one?

monsieurtanuki · 2024-06-16T06:09:36Z

packages/smooth_app/lib/pages/product/edit_ocr_page.dart

+  OcrState _extractState(TransientFile transientFile) {
+    if (_extractingData) {
+      return OcrState.EXTRACTING_DATA;
+    } else if (transientFile.isServerImage()) {
+      return OcrState.IMAGE_LOADED;
+    } else if (transientFile.isImageAvailable()) {
+      return OcrState.IMAGE_LOADING;
+    } else {
+      return OcrState.OTHER;
+    }
+  }
+}


Looks brillant!

monsieurtanuki · 2024-06-16T06:12:58Z

packages/smooth_app/lib/pages/product/edit_ocr_page.dart

+              const CircularProgressIndicator.adaptive(),
+              Expanded(
+                child: Text(
+                  helper.getActionExtractingData(appLocalizations),
+                  textAlign: TextAlign.center,
+                ),
+              ),
+              const CircularProgressIndicator.adaptive(),


2 progress indicators? A bit too much, isn't it?

You can see it, on the video, with the Shimmer effect, actually it's OK.
To be honest, I've not checked on Android and in that case, I will remove the first one on non-Apple devices

monsieurtanuki · 2024-06-16T06:13:36Z

packages/smooth_app/lib/pages/product/edit_ocr_page.dart

+              ),
+              const Icon(
+                Icons.download,
+                semanticLabel: '',


Is that worth it?

Yes, otherwise, the screen reader tries to say the character in font (that's the default behavior)

Yes, otherwise, the screen reader tries to say the character in font (that's the default behavior)

Hmm, I guess that wouldn't hurt to say "download".
If I've understood well, that also means that we're supposed to give each Icon we use in the app that meaningless semanticLabel: '' parameter, which is not nice.

That's not needed, because we have the text just before.

monsieurtanuki · 2024-06-16T06:16:36Z

packages/smooth_app/lib/pages/product/edit_ocr_page.dart

I know it's your way of coding, putting several classes in the same file, and my way of coding is rather one class per file.
Beyond those differences, you may consider that 600 lines of code is a bit too much for a single file, and create additional files, e.g. one per button.

Ok, I will move _EditOcrMainAction in a separate file, but sub widgets are just a way to prevent of giant Widget

I'm ok with subwidgets, I just don't like big files - and giant widgets ;)

monsieurtanuki · 2024-06-16T06:18:31Z

packages/smooth_app/lib/pages/product/edit_ocr_page.dart

-    } catch (e) {
-      //
-    }
+    } catch (_) {}


So you're not resetting _extractingData to false when there's an exception in getExtractedText?

Nice catch 😆
I will fix that

teolemon

I would potentially add the language the photo is in, and the language it's extracting to in the loading message.
Alternatively we can shift to a tab based UI instead of dropdowns

Extract ingredients/packaging: loading / loaded / extracting states

fb2d1ee

g123k added ingredients ✏️ Editing - Packaging input Related to the structured input of food packaging. ✏️ Editing - ingredients labels Jun 15, 2024

g123k self-assigned this Jun 15, 2024

g123k requested a review from a team as a code owner June 15, 2024 20:31

github-actions bot added 🥫 Product page ✏️ Editing - 📦 Packaging input labels Jun 15, 2024

g123k added 3 commits June 15, 2024 22:32

Extractingdata variable can be private

a685edd

Tiny perf improvement

1c799bf

Fix Lint

d854301

monsieurtanuki reviewed Jun 16, 2024

View reviewed changes

teolemon approved these changes Jun 16, 2024

View reviewed changes

teolemon merged commit 65ce9c4 into openfoodfacts:develop Jun 16, 2024
6 checks passed

github-actions bot mentioned this pull request Jun 16, 2024

chore(develop): release 4.15.0 #5106

Merged

This was referenced Jun 17, 2024

chore: OCR page: missing case fixed + file splited #5395

Merged

Make the OCR button Invisible until the OCR is complete #3975

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Extract ingredients/packaging: loading / loaded / extracting states #5384

feat: Extract ingredients/packaging: loading / loaded / extracting states #5384

g123k commented Jun 15, 2024

codecov-commenter commented Jun 15, 2024

monsieurtanuki left a comment

monsieurtanuki Jun 16, 2024

g123k Jun 16, 2024

monsieurtanuki Jun 16, 2024

monsieurtanuki Jun 16, 2024

g123k Jun 16, 2024

monsieurtanuki Jun 16, 2024

g123k Jun 16, 2024

monsieurtanuki Jun 16, 2024

g123k Jun 16, 2024

monsieurtanuki Jun 16, 2024

g123k Jun 16, 2024

monsieurtanuki Jun 16, 2024

monsieurtanuki Jun 16, 2024

g123k Jun 16, 2024

teolemon left a comment

feat: Extract ingredients/packaging: loading / loaded / extracting states #5384

feat: Extract ingredients/packaging: loading / loaded / extracting states #5384

Conversation

g123k commented Jun 15, 2024

codecov-commenter commented Jun 15, 2024

Codecov Report

monsieurtanuki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teolemon left a comment

Choose a reason for hiding this comment