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

fix: 4219 - check if new picture is big enough before server upload #4224

Merged
merged 1 commit into from
Jun 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
60 changes: 54 additions & 6 deletions packages/smooth_app/lib/background/background_task_image.dart
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,14 @@ class BackgroundTaskImage extends BackgroundTaskUpload {
return result;
}

// cf. https://github.com/openfoodfacts/smooth-app/issues/4219
// TODO(monsieurtanuki): move to off-dart
static const int minimumWidth = 640;
static const int minimumHeight = 160;

static bool isPictureBigEnough(final num width, final num height) =>
width >= minimumWidth || height >= minimumHeight;

/// Adds the background task about uploading a product image.
static Future<void> addTask(
final String barcode, {
Expand Down Expand Up @@ -213,19 +221,54 @@ class BackgroundTaskImage extends BackgroundTaskUpload {
/// Conversion factor to `int` from / to UI / background task.
static const int cropConversionFactor = 1000000;

/// Returns true if a cropped operation is needed - after having performed it.
Future<bool> _crop(final File file) async {
/// Returns true if a crop operation is needed - after having performed it.
///
/// Returns false if no crop operation is needed.
/// Returns null if the image (cropped or not) is too small.
Future<bool?> _crop(final File file) async {
final ui.Image full = await loadUiImage(await File(fullPath).readAsBytes());
if (cropX1 == 0 &&
cropY1 == 0 &&
cropX2 == cropConversionFactor &&
cropY2 == cropConversionFactor &&
rotationDegrees == 0) {
if (!isPictureBigEnough(full.width, full.height)) {
return null;
}
// in that case, no need to crop
return false;
}
final ui.Image full = await loadUiImage(
await File(fullPath).readAsBytes(),
);

Size getCroppedSize() {
final Rect cropRect = getResizedRect(
Rect.fromLTRB(
cropX1.toDouble(),
cropY1.toDouble(),
cropX2.toDouble(),
cropY2.toDouble(),
),
1 / cropConversionFactor,
);
switch (CropRotationExtension.fromDegrees(rotationDegrees)!) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI in that kind of situation a return switch from Dart 3 may look (a bit) better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI in that kind of situation a return switch from Dart 3 may look (a bit) better

@g123k Fair enough!
I wouldn't say it looks better (just a matter of taste), but thank you for reminding me the new dart syntax!

case CropRotation.up:
case CropRotation.down:
return Size(
cropRect.width * full.height,
cropRect.height * full.width,
);
case CropRotation.left:
case CropRotation.right:
return Size(
cropRect.width * full.width,
cropRect.height * full.height,
);
}
}

final Size croppedSize = getCroppedSize();
if (!isPictureBigEnough(croppedSize.width, croppedSize.height)) {
return null;
}
final ui.Image cropped = await CropController.getCroppedBitmap(
crop: getResizedRect(
Rect.fromLTRB(
Expand Down Expand Up @@ -253,7 +296,12 @@ class BackgroundTaskImage extends BackgroundTaskUpload {
Future<void> upload() async {
final String path;
final String croppedPath = _getCroppedPath();
if (await _crop(File(croppedPath))) {
final bool? neededCrop = await _crop(File(croppedPath));
if (neededCrop == null) {
// TODO(monsieurtanuki): maybe something more refined when we dismiss the picture, like alerting the user, though it's not supposed to happen anymore from upstream.
return;
}
if (neededCrop) {
path = croppedPath;
} else {
path = fullPath;
Expand Down
22 changes: 22 additions & 0 deletions packages/smooth_app/lib/l10n/app_en.arb
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,28 @@
"@crop_page_action_local": {
"description": "Action being performed on the crop page"
},
"crop_page_too_small_image_title": "The image is too small!",
"@crop_page_too_small_image_title": {
"description": "Title of a dialog warning the user that the image is too small for upload"
},
"crop_page_too_small_image_message": "The minimum size in pixels for picture upload is {expectedMinWidth}x{expectedMinHeight}. The current picture is {actualWidth}x{actualHeight}.",
"@crop_page_too_small_image_message": {
"description": "Message of a dialog warning the user that the image is too small for upload",
"placeholders": {
"expectedMinWidth": {
"type": "int"
},
"expectedMinHeight": {
"type": "int"
},
"actualWidth": {
"type": "int"
},
"actualHeight": {
"type": "int"
}
}
},
"crop_page_action_server": "Preparing a call to the server…",
"@crop_page_action_server": {
"description": "Action being performed on the crop page"
Expand Down
78 changes: 69 additions & 9 deletions packages/smooth_app/lib/pages/crop_page.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import 'package:smooth_app/data_models/continuous_scan_model.dart';
import 'package:smooth_app/database/dao_int.dart';
import 'package:smooth_app/database/local_database.dart';
import 'package:smooth_app/generic_lib/design_constants.dart';
import 'package:smooth_app/generic_lib/dialogs/smooth_alert_dialog.dart';
import 'package:smooth_app/generic_lib/loading_dialog.dart';
import 'package:smooth_app/helpers/database_helper.dart';
import 'package:smooth_app/helpers/image_compute_container.dart';
Expand Down Expand Up @@ -243,6 +244,61 @@ class _CropPageState extends State<CropPage> {
}

Future<File?> _saveFileAndExitTry() async {
final AppLocalizations appLocalizations = AppLocalizations.of(context);

// only for new image upload we have to check the minimum size.
if (widget.imageId == null) {
// Returns the size of the resulting cropped image.
Size getCroppedSize() {
switch (_controller.rotation) {
case CropRotation.up:
case CropRotation.down:
return Size(
_controller.crop.width * _image.width,
_controller.crop.height * _image.height,
);
case CropRotation.left:
case CropRotation.right:
return Size(
_controller.crop.width * _image.height,
_controller.crop.height * _image.width,
);
}
}

final Size croppedSize = getCroppedSize();
if (!BackgroundTaskImage.isPictureBigEnough(
croppedSize.width,
croppedSize.height,
)) {
final int width = croppedSize.width.floor();
final int height = croppedSize.height.floor();
await showDialog<void>(
context: context,
builder: (BuildContext context) => SmoothAlertDialog(
title: appLocalizations.crop_page_too_small_image_title,
body: Text(
appLocalizations.crop_page_too_small_image_message(
BackgroundTaskImage.minimumWidth,
BackgroundTaskImage.minimumHeight,
width,
height,
),
),
actionsAxis: Axis.vertical,
positiveAction: SmoothActionButton(
text: appLocalizations.okay,
onPressed: () => Navigator.of(context).pop(),
),
),
);
return null;
}
}

if (!mounted) {
return null;
}
final LocalDatabase localDatabase = context.read<LocalDatabase>();
final DaoInt daoInt = DaoInt(localDatabase);
final int sequenceNumber =
Expand All @@ -255,7 +311,7 @@ class _CropPageState extends State<CropPage> {
);

setState(
() => _progress = AppLocalizations.of(context).crop_page_action_server,
() => _progress = appLocalizations.crop_page_action_server,
);
if (widget.imageId == null) {
// in this case, it's a brand new picture, with crop parameters.
Expand Down Expand Up @@ -312,21 +368,26 @@ class _CropPageState extends State<CropPage> {
return croppedFile;
}

Future<void> _saveFileAndExit() async {
Future<bool> _saveFileAndExit() async {
setState(
() => _progress = AppLocalizations.of(context).crop_page_action_saving,
);
try {
final File? file = await _saveFileAndExitTry();
_progress = null;
if (!mounted) {
return;
}
if (file == null) {
setState(() {});
if (mounted) {
setState(() {});
}
return false;
} else {
Navigator.of(context).pop<File>(file);
if (mounted) {
Navigator.of(context).pop<File>(file);
}
return true;
}
} catch (e) {
return false;
} finally {
_progress = null;
}
Expand Down Expand Up @@ -420,8 +481,7 @@ class _CropPageState extends State<CropPage> {
}

try {
await _saveFileAndExit();
return true;
return _saveFileAndExit();
} catch (e) {
if (mounted) {
// not likely to happen, but you never know...
Expand Down
4 changes: 2 additions & 2 deletions packages/smooth_app/pubspec.lock
Original file line number Diff line number Diff line change
Expand Up @@ -305,10 +305,10 @@ packages:
dependency: "direct main"
description:
name: crop_image
sha256: "0b5b939106eb71cbb66d2b97e56d9ec3469d88b12324143439bf0911e4c9bf11"
sha256: "2c70294bdcb0f2fd8b7d660535b314eb5d2477000057e1f30db8295d1588c422"
url: "https://pub.dev"
source: hosted
version: "1.0.6"
version: "1.0.10"
cross_file:
dependency: transitive
description:
Expand Down
2 changes: 1 addition & 1 deletion packages/smooth_app/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ dependencies:
flutter_native_splash: 2.2.19
image: ^4.0.17
auto_size_text: 3.0.0
crop_image: 1.0.6
crop_image: 1.0.10
shared_preferences: 2.1.1
intl: 0.18.0
collection: 1.17.1
Expand Down