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

refactor: ProductImageData to contain all image links #3088

Merged
merged 12 commits into from
Oct 12, 2022
Merged

refactor: ProductImageData to contain all image links #3088

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
98a717f
refactor: ProductImageData to contain all image links
WildOrangutan Oct 3, 2022
ce5a900
Revert "refactor: ProductImageData to contain all image links"
WildOrangutan Oct 6, 2022
faaeadb
refactor: ProductImageData imageUrl to ImageDescriptor
WildOrangutan Oct 6, 2022
dd5f9c4
refactor: Move _getProductImageData() to factory method
WildOrangutan Oct 7, 2022
85a2f26
refactor: Revert part of ImageDescriptor changes
WildOrangutan Oct 7, 2022
1014e98
feat: ProductImageData getImageUrl
WildOrangutan Oct 7, 2022
ac75b2a
fix: Gallery product image quality
WildOrangutan Oct 7, 2022
cf7c9fd
draft: Fallback is full image doesn't exist
WildOrangutan Oct 10, 2022
cc2f3a0
revert: Partial revert of fallback draft
WildOrangutan Oct 11, 2022
0f2db3d
feat: Show error when image edit fails
WildOrangutan Oct 11, 2022
2bb8620
fix: Fallback to other image qualities
WildOrangutan Oct 12, 2022
68b8702
fix: Remove other image sizes?
WildOrangutan Oct 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/smooth_app/lib/cards/data_cards/image_upload_card.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,11 @@ class _ImageUploadCardState extends State<ImageUploadCard> {
// we need to load the full resolution image

if (_imageFullProvider == null) {
final String imageFullUrl =
widget.productImageData.imageUrl!.replaceAll('.400.', '.full.');
_imageFullProvider = NetworkImage(imageFullUrl);
final String? imageFullUrl =
widget.productImageData.getImageUrl(ImageSize.ORIGINAL);
if (imageFullUrl != null) {
_imageFullProvider = NetworkImage(imageFullUrl);
}
}

// TODO(monsieurtanuki): careful, waiting for pop'ed value
Expand Down
39 changes: 38 additions & 1 deletion packages/smooth_app/lib/data_models/product_image_data.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import 'package:openfoodfacts/model/ProductImage.dart';
import 'package:openfoodfacts/openfoodfacts.dart';

class ProductImageData {
const ProductImageData({
Expand All @@ -8,8 +8,45 @@ class ProductImageData {
this.imageUrl,
});

factory ProductImageData.from(ProductImage image, String barcode) {
return ProductImageData(
imageField: image.field,
// TODO(VaiTon): i18n
title: image.imgid ?? '',
buttonText: image.imgid ?? '',
imageUrl: ImageHelper.buildUrl(barcode, image),
);
}

final ImageField imageField;
final String title;
final String buttonText;
final String? imageUrl;

/// Try to convert [imageUrl] to specified [size].
/// Note that url for specified [size] might not exist on API.
String? getImageUrl(ImageSize size) {
final String? imageUrl = this.imageUrl;
if (imageUrl == null) {
return null;
}

const String SEPARATOR = '.';

final int extensionIndex = imageUrl.lastIndexOf(SEPARATOR);
if (extensionIndex < 0) {
return null;
}

final int sizeIndex = imageUrl.lastIndexOf(SEPARATOR, extensionIndex - 1);
if (sizeIndex < 0) {
return null;
}

final String baseUrl = imageUrl.substring(0, sizeIndex + 1);
final String number = size.toNumber();
final String extension =
imageUrl.substring(extensionIndex, imageUrl.length);
return baseUrl + number + extension;
}
}
5 changes: 3 additions & 2 deletions packages/smooth_app/lib/generic_lib/widgets/images/smooth_images_sliver_grid.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,12 @@ class SmoothImagesSliverGrid extends SmoothImagesView {
final MapEntry<ProductImageData, ImageProvider<Object>?> entry =
imageList[index];
final ImageProvider? imageProvider = entry.value;
final String? imageUrl = entry.key.imageUrl;

return imageProvider == null
return imageProvider == null || imageUrl == null
? const PictureNotFound()
: Hero(
tag: entry.key.imageUrl!,
tag: imageUrl,
child: _ImageTile(
image: imageProvider,
onTap: onTap == null
Expand Down
8 changes: 8 additions & 0 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 @@ -1698,5 +1698,13 @@
"word_separator": ", ",
"@word_separator": {
"description": "Word separator string. In English, this is a comma followed by a space: ', '"
},
"image_download_error": "Failed to download image",
"@image_download_error": {
"description": "Error message, when image download fails"
},
"image_edit_url_error": "Failed to edit image, image url not set.",
"@image_edit_url_error": {
"description": "Error message, when editing image fails, due to missing url."
}
}
24 changes: 13 additions & 11 deletions packages/smooth_app/lib/pages/product/product_image_gallery_view.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,8 +227,13 @@ class _ProductImageGalleryViewState extends State<ProductImageGalleryView> {
}

Future<Iterable<ProductImageData>?> _getProductImages() async {
final String? barcode = widget.product.barcode;
if (barcode == null) {
return null;
}

final ProductQueryConfiguration configuration = ProductQueryConfiguration(
widget.product.barcode!,
barcode,
fields: <ProductField>[ProductField.IMAGES],
language: ProductQuery.getLanguage(),
country: ProductQuery.getCountry(),
Expand All @@ -250,7 +255,8 @@ class _ProductImageGalleryViewState extends State<ProductImageGalleryView> {
return null;
}

return _deduplicateImages(resultProduct.images!).map(_getProductImageData);
return _deduplicateImages(resultProduct.images!)
.map((ProductImage image) => ProductImageData.from(image, barcode));
}

/// Groups the list of [ProductImage] by [ProductImage.imgid]
Expand All @@ -259,15 +265,11 @@ class _ProductImageGalleryViewState extends State<ProductImageGalleryView> {
images
.groupListsBy((ProductImage element) => element.imgid)
.values
.map((List<ProductImage> sameIdImages) => sameIdImages.firstOrNull)
.map(_selectProductImage)
.whereNotNull();

/// Created a [ProductImageData] from a [ProductImage]
ProductImageData _getProductImageData(ProductImage image) => ProductImageData(
imageField: image.field,
// TODO(VaiTon): i18n
title: image.imgid ?? '',
buttonText: image.imgid ?? '',
imageUrl: ImageHelper.buildUrl(widget.product.barcode, image),
);
ProductImage? _selectProductImage(Iterable<ProductImage> images) =>
images.firstWhereOrNull(
(ProductImage image) => image.size == ImageSize.DISPLAY) ??
Copy link
Contributor

Choose a reason for hiding this comment

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

@WildOrangutan So if I sum up, that's where you deduplicate by restricting to 400 pictures only, right?

I would rename _selectProductImage to _selectBestProductImage, and/or add a comment.
I believe the point of this method should be to take the best picture: 400, or 200, or 100, and that's not exactly what is in your code (you just say 400 or else whatever).
Perhaps you could rewrite it in more than one line to make it explicit (we have a method here, we have a bit of space): that would help the code reviewer now (it's me) and whoever will maintain the code later.
You can even write it in one line:

images.firstWhereOrNull(
           (ProductImage image) => image.size == ImageSize.DISPLAY) ??
images.firstWhereOrNull(
           (ProductImage image) => image.size == ImageSize.SMALL) ??
images.firstWhereOrNull(
           (ProductImage image) => image.size == ImageSize.THUMB)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if I sum up, that's where you deduplicate by restricting to 400 pictures only, right?
Yes, this is where I selected 400 as middle ground between two screens (1st and 2nd screenshot).

I've updated the code, hope I didn't complicate things to much, by using map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, what about ORIGINAL, UNKOWN or null image sizes? Isn't it better for image load too slow, other than not load at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

@WildOrangutan If for some reason we have to download hundreds of "other" ORIGINAL, that can be ugly, performance-wise. Could even crash the app I guess.

But

  • that's theoretical, as we can expect 400 versions everywhere
  • in your latest code no additional comment is needed: anybody that reads it can understand the priorities and - if relevant later - clearly remove one picture version or reorder them (e.g. "200 is good enough" or "never full or unknown").

So let's go with it!

images.firstOrNull;
}
30 changes: 23 additions & 7 deletions packages/smooth_app/lib/pages/product/product_image_viewer.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import 'dart:io';
import 'package:flutter/material.dart';
import 'package:flutter_gen/gen_l10n/app_localizations.dart';
import 'package:http/http.dart' as http;
import 'package:openfoodfacts/model/ProductImage.dart';
import 'package:path_provider/path_provider.dart';
import 'package:photo_view/photo_view.dart';
import 'package:provider/provider.dart';
Expand Down Expand Up @@ -32,6 +33,7 @@ class ProductImageViewer extends StatefulWidget {

class _ProductImageViewerState extends State<ProductImageViewer> {
late final ProductImageData imageData;
late final AppLocalizations appLocalizations = AppLocalizations.of(context);

/// When the image is edited, this is the new image
late ImageProvider imageProvider;
Expand All @@ -50,7 +52,7 @@ class _ProductImageViewerState extends State<ProductImageViewer> {
extendBodyBehindAppBar: true,
backgroundColor: Colors.black,
floatingActionButton: FloatingActionButton.extended(
label: Text(AppLocalizations.of(context).edit_photo_button_label),
label: Text(appLocalizations.edit_photo_button_label),
icon: const Icon(Icons.edit),
backgroundColor: Theme.of(context).colorScheme.primary,
onPressed: () {
Expand Down Expand Up @@ -90,12 +92,17 @@ class _ProductImageViewerState extends State<ProductImageViewer> {
);

Future<void> _editImage(final DaoInt daoInt) async {
final File? imageFile = await LoadingDialog.run<File>(
context: context,
future: _downloadImageFile(daoInt, imageData.imageUrl!),
);
final String? imageUrl = imageData.getImageUrl(ImageSize.ORIGINAL);
if (imageUrl == null) {
await _showDownloadFailedDialog(appLocalizations.image_edit_url_error);
return;
}

final File? imageFile = await LoadingDialog.run<File?>(
context: context, future: _downloadImageFile(daoInt, imageUrl));

if (imageFile == null) {
await _showDownloadFailedDialog(appLocalizations.image_download_error);
return;
}

Expand Down Expand Up @@ -125,10 +132,19 @@ class _ProductImageViewerState extends State<ProductImageViewer> {
}
}

Future<void> _showDownloadFailedDialog(String? title) =>
LoadingDialog.error(context: context, title: title);

static const String _CROP_IMAGE_SEQUENCE_KEY = 'crop_image_sequence';

Future<File> _downloadImageFile(DaoInt daoInt, String url) async {
final http.Response response = await http.get(Uri.parse(url));
Future<File?> _downloadImageFile(DaoInt daoInt, String url) async {
final Uri uri = Uri.parse(url);
final http.Response response = await http.get(uri);
final int code = response.statusCode;
if (code != 200) {
throw NetworkImageLoadException(statusCode: code, uri: uri);
}

final Directory tempDirectory = await getTemporaryDirectory();

final int sequenceNumber =
Expand Down