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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Jun 16, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb2d1ee
Extract ingredients/packaging: loading / loaded / extracting states
g123k Jun 15, 2024
a685edd
Extractingdata variable can be private
g123k Jun 15, 2024
1c799bf
Tiny perf improvement
g123k Jun 15, 2024
d854301
Fix Lint
g123k Jun 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions packages/smooth_app/lib/l10n/app_en.arb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

"@edit_ingredients_extract_ingredients_btn_text": {
"description": "Ingredients edition - Extract ingredients"
},
"edit_ingredients_extracting_ingredients_btn_text": "Extracting ingredients\nfrom the photo",
"@edit_ingredients_extracting_ingredients_btn_text": {
"description": "Ingredients edition - Extracting ingredients"
},
"edit_ingredients_loading_photo_btn_text": "Loading photo…",
"@edit_ingredients_loading_photo_btn_text": {
"description": "Ingredients edition - Loading photo from the server"
},
"edit_ingredients_refresh_photo_btn_text": "Refresh photo",
"@edit_ingredients_refresh_photo_btn_text": {
"description": "Ingredients edition - Refresh photo"
},
"edit_packaging_extract_btn_text": "Extract packaging",
"edit_packaging_extract_btn_text": "Extract packaging\nfrom the photo",
"@edit_packaging_extract_btn_text": {
"description": "Packaging edition - OCR-Extract packaging"
},
"edit_packaging_extracting_btn_text": "Extracting packaging from the photo",
"@edit_packaging_extracting_btn_text": {
"description": "Packaging edition - OCR-Extracting packaging"
},
"edit_packaging_loading_photo_btn_text": "Loading photo…",
"@edit_packaging_loading_photo_btn_text": {
"description": "Packaging edition - Loading photo from the server"
},
"edit_packaging_refresh_photo_btn_text": "Refresh photo",
"@edit_packaging_refresh_photo_btn_text": {
"description": "Packaging edition - Refresh photo"
Expand Down
248 changes: 233 additions & 15 deletions packages/smooth_app/lib/pages/product/edit_ocr_page.dart
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ 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:shimmer/shimmer.dart';
import 'package:smooth_app/background/background_task_details.dart';
import 'package:smooth_app/data_models/up_to_date_mixin.dart';
import 'package:smooth_app/database/local_database.dart';
Expand All @@ -18,6 +19,7 @@ import 'package:smooth_app/pages/product/explanation_widget.dart';
import 'package:smooth_app/pages/product/multilingual_helper.dart';
import 'package:smooth_app/pages/product/ocr_helper.dart';
import 'package:smooth_app/pages/product/product_image_button.dart';
import 'package:smooth_app/themes/smooth_theme_colors.dart';
import 'package:smooth_app/widgets/smooth_scaffold.dart';

/// Editing with OCR a product field and the corresponding image.
Expand All @@ -43,6 +45,7 @@ class _EditOcrPageState extends State<EditOcrPage> with UpToDateMixin {
late final MultilingualHelper _multilingualHelper;

OcrHelper get _helper => widget.helper;
bool _extractingData = false;

@override
void initState() {
Expand All @@ -62,7 +65,8 @@ class _EditOcrPageState extends State<EditOcrPage> with UpToDateMixin {
///
/// When done, populates the related page field.
Future<void> _extractData() async {
// TODO(monsieurtanuki): hide the "extract" button while extracting, or display a loading dialog on top
setState(() => _extractingData = true);

try {
final String? extractedText = await _helper.getExtractedText(
widget.product,
Expand All @@ -72,6 +76,8 @@ class _EditOcrPageState extends State<EditOcrPage> with UpToDateMixin {
return;
}

setState(() => _extractingData = false);

if (extractedText == null || extractedText.isEmpty) {
await LoadingDialog.error(
context: context,
Expand All @@ -83,9 +89,7 @@ class _EditOcrPageState extends State<EditOcrPage> with UpToDateMixin {
if (_controller.text != extractedText) {
setState(() => _controller.text = extractedText);
}
} catch (e) {
//
}
} catch (_) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch 😆
I will fix that

}

/// Updates the product field on the server.
Expand Down Expand Up @@ -282,17 +286,11 @@ class _EditOcrPageState extends State<EditOcrPage> with UpToDateMixin {
setState: setState,
product: upToDateProduct,
),
if (transientFile.isServerImage())
SmoothActionButtonsBar.single(
action: SmoothActionButton(
text:
_helper.getActionExtractText(appLocalizations),
onPressed: () async => _extractData(),
),
)
else if (transientFile.isImageAvailable())
// TODO(monsieurtanuki): what if slow upload? text instead?
const CircularProgressIndicator.adaptive(),
_EditOcrMainAction(
onPressed: _extractData,
helper: _helper,
state: _extractState(transientFile),
),
const SizedBox(height: MEDIUM_SPACE),
TextField(
controller: _controller,
Expand Down Expand Up @@ -377,4 +375,224 @@ class _EditOcrPageState extends State<EditOcrPage> with UpToDateMixin {
}
return result;
}

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;
}
}
}
Comment on lines +379 to +390
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks brillant!


class _EditOcrMainAction extends StatelessWidget {
const _EditOcrMainAction({
required this.onPressed,
required this.helper,
required this.state,
});

final VoidCallback onPressed;
final OcrHelper helper;
final OcrState state;

@override
Widget build(BuildContext context) {
final AppLocalizations appLocalizations = AppLocalizations.of(context);

final Widget? child = switch (state) {
OcrState.IMAGE_LOADING => _EditOcrActionLoadingContent(
helper: helper,
appLocalizations: appLocalizations,
),
OcrState.IMAGE_LOADED => _ExtractMainActionContentLoaded(
helper: helper,
appLocalizations: appLocalizations,
onPressed: onPressed,
),
OcrState.EXTRACTING_DATA => _EditOcrActionExtractingContent(
helper: helper,
appLocalizations: appLocalizations,
),
OcrState.OTHER => null,
};

if (child == null) {
return EMPTY_WIDGET;
}

final SmoothColorsThemeExtension theme =
Theme.of(context).extension<SmoothColorsThemeExtension>()!;

return SizedBox(
height: 45.0 * (_computeFontScaleFactor(context)),
width: double.infinity,
child: DecoratedBox(
decoration: BoxDecoration(
borderRadius: ANGULAR_BORDER_RADIUS,
color: theme.primarySemiDark,
border: Border.all(
color: theme.primaryBlack,
width: 2.0,
),
),
child: Material(
type: MaterialType.transparency,
child: ProgressIndicatorTheme(
data: const ProgressIndicatorThemeData(
color: Colors.white,
),
child: IconTheme(
data: const IconThemeData(color: Colors.white),
child: DefaultTextStyle(
style: const TextStyle(
fontWeight: FontWeight.bold,
fontSize: 15.5,
color: Colors.white,
),
child: child,
),
),
),
),
),
);
}

double _computeFontScaleFactor(BuildContext context) {
final double fontSize = DefaultTextStyle.of(context).style.fontSize ?? 15.0;
final double scaledFontSize =
MediaQuery.textScalerOf(context).scale(fontSize);

return scaledFontSize / fontSize;
}
}

class _EditOcrActionExtractingContent extends StatelessWidget {
const _EditOcrActionExtractingContent({
required this.helper,
required this.appLocalizations,
});

final OcrHelper helper;
final AppLocalizations appLocalizations;

@override
Widget build(BuildContext context) {
return Semantics(
label: helper.getActionLoadingPhoto(appLocalizations),
excludeSemantics: true,
child: Shimmer(
gradient: const LinearGradient(
begin: Alignment.centerLeft,
end: Alignment.centerRight,
colors: <Color>[
Colors.black,
Colors.white,
Colors.black,
],
),
child: Padding(
padding: const EdgeInsets.symmetric(horizontal: MEDIUM_SPACE),
child: Row(
mainAxisAlignment: MainAxisAlignment.center,
crossAxisAlignment: CrossAxisAlignment.center,
children: <Widget>[
const CircularProgressIndicator.adaptive(),
Expanded(
child: Text(
helper.getActionExtractingData(appLocalizations),
textAlign: TextAlign.center,
),
),
const CircularProgressIndicator.adaptive(),
Comment on lines +505 to +512
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

],
),
),
),
);
}
}

class _ExtractMainActionContentLoaded extends StatelessWidget {
const _ExtractMainActionContentLoaded({
required this.helper,
required this.appLocalizations,
required this.onPressed,
});

final OcrHelper helper;
final AppLocalizations appLocalizations;
final VoidCallback onPressed;

@override
Widget build(BuildContext context) {
return Semantics(
excludeSemantics: true,
value: helper.getActionExtractText(appLocalizations),
button: true,
child: InkWell(
onTap: onPressed,
borderRadius: ANGULAR_BORDER_RADIUS,
child: Padding(
padding: const EdgeInsets.symmetric(horizontal: MEDIUM_SPACE),
child: Row(
mainAxisAlignment: MainAxisAlignment.center,
crossAxisAlignment: CrossAxisAlignment.center,
children: <Widget>[
Expanded(
child: Text(
helper.getActionExtractText(appLocalizations),
),
),
const Icon(
Icons.download,
semanticLabel: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that worth it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

),
],
),
),
),
);
}
}

class _EditOcrActionLoadingContent extends StatelessWidget {
const _EditOcrActionLoadingContent({
required this.helper,
required this.appLocalizations,
});

final OcrHelper helper;
final AppLocalizations appLocalizations;

@override
Widget build(BuildContext context) {
return Padding(
padding: const EdgeInsets.symmetric(horizontal: MEDIUM_SPACE),
child: Row(
mainAxisAlignment: MainAxisAlignment.center,
crossAxisAlignment: CrossAxisAlignment.center,
children: <Widget>[
Expanded(
child: Text(
helper.getActionLoadingPhoto(appLocalizations),
),
),
const CircularProgressIndicator.adaptive(),
],
),
);
}
}

enum OcrState {
IMAGE_LOADING,
IMAGE_LOADED,
EXTRACTING_DATA,
OTHER,
}
6 changes: 6 additions & 0 deletions packages/smooth_app/lib/pages/product/ocr_helper.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ abstract class OcrHelper {
/// Returns the "extract text" button label
String getActionExtractText(final AppLocalizations appLocalizations);

/// Returns the "loading photo" button label
String getActionLoadingPhoto(final AppLocalizations appLocalizations);

/// Returns the "Extracting data…" button label
String getActionExtractingData(final AppLocalizations appLocalizations);

/// Returns the "refresh photo" button label
String getActionRefreshPhoto(final AppLocalizations appLocalizations);

Expand Down
10 changes: 9 additions & 1 deletion packages/smooth_app/lib/pages/product/ocr_ingredients_helper.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,15 @@ class OcrIngredientsHelper extends OcrHelper {

@override
String getActionExtractText(final AppLocalizations appLocalizations) =>
appLocalizations.edit_ingredients_extrait_ingredients_btn_text;
appLocalizations.edit_ingredients_extract_ingredients_btn_text;

@override
String getActionExtractingData(AppLocalizations appLocalizations) =>
appLocalizations.edit_ingredients_extracting_ingredients_btn_text;

@override
String getActionLoadingPhoto(AppLocalizations appLocalizations) =>
appLocalizations.edit_ingredients_loading_photo_btn_text;

@override
String getActionRefreshPhoto(final AppLocalizations appLocalizations) =>
Expand Down
8 changes: 8 additions & 0 deletions packages/smooth_app/lib/pages/product/ocr_packaging_helper.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,14 @@ class OcrPackagingHelper extends OcrHelper {
String getActionExtractText(final AppLocalizations appLocalizations) =>
appLocalizations.edit_packaging_extract_btn_text;

@override
String getActionExtractingData(AppLocalizations appLocalizations) =>
appLocalizations.edit_packaging_extracting_btn_text;

@override
String getActionLoadingPhoto(AppLocalizations appLocalizations) =>
appLocalizations.edit_packaging_loading_photo_btn_text;

@override
String getActionRefreshPhoto(final AppLocalizations appLocalizations) =>
appLocalizations.edit_packaging_refresh_photo_btn_text;
Expand Down
Loading