refactor: ProductImageData to contain all image links #3088
Conversation
|
Heyy @WildOrangutan thanks for this submission, this turns out to be harder then I expected it. I don't know if you noticed but we have all connections to the server extracted to a own package on pub.dev (Here is the repo), so we have full controll over all the models we use. So if we'd use this approach we could instead create a own class with all the images in all sizes. But while looking at the implementation I stumbled across static String? buildUrl(
final String? barcode,
final ProductImage image, {
final QueryType? queryType,
}) =>
barcode == null
? null
: '${getProductImageRootUrl(barcode, queryType: queryType)}/${image.field.value}_${image.language.code}.${image.rev}.${image.size.toNumber()}.jpg';I guess it should be possible to get all the different sizes of a image when we modify this to allow to overwrite the size. c.f. But I'd love to hear @monsieurtanuki's opinion on that, He's the one who coded this image part |
|
@M123-dev I think you're right: if it's just a matter of getting the same image with a different definition, you can reverse-engineer the url and create the new one. static String buildUrlFromRelative(
final String sourceUrl, // e.g. https://static.openfoodfacts.org/images/products/359/671/046/2858/front_fr.4.200.jpg
final ImageSize destinationImageSize, // e.g. ImageSize.ORIGINAL
) // in that case it should return https://static.openfoodfacts.org/images/products/359/671/046/2858/front_fr.4.full.jpg |
|
Does backend api guarantee that all image sizes exist? I think building urls on frontend might not be the most robust and flexible solution. |
@stephanegigandet Question:
|
|
@WildOrangutan Actually we're already doing it in if (_imageFullProvider == null) {
final String imageFullUrl =
widget.productImageData.imageUrl!.replaceAll('.400.', '.full.');
_imageFullProvider = NetworkImage(imageFullUrl);
}
`` |
|
Doing What if I change it in following way: class ProductImageData {
// Initialize in constructor from **ANY OF** `ProductImage`
final String _baseImageUrl;
String getImageUrl(ImageSize size) {
return "$_baseImageUrl.${image.size.toNumber()}.jpg
}
} |
|
@WildOrangutan My point here is that it's just about replacing the last part of the url (regardless of the method to do it).
I understand your point about regexp, and there's a cleaner way to do that, like truncating the url just after the before-last dot, and concatenate
class ProductImageData {
// Initialize in constructor from **ANY OF** `ProductImage`
final String _baseImageUrl;I would definitely not do that in a first approach for different reasons, among which:
So, what about a clean static method that calls static String? buildUrlFromRelative(
final String sourceUrl, // e.g. https://static.openfoodfacts.org/images/products/359/671/046/2858/front_fr.4.200.jpg
final ImageSize destinationImageSize, // e.g. ImageSize.ORIGINAL
int pos1 = sourceUrl.lastIndexOf('.');
if (pos1 < 0) {
return null;
}
int pos2 = sourceUrl.lastIndexOf('.', pos1);
if (pos2 < 0) {
return null;
}
return '${sourceUrl.substring(0, pos2)}.${destinationImageSize.toNumber()}.jpg';
} |
|
Since How do last changes look like? That's what I had in mind. |
Codecov Report
@@ Coverage Diff @@
## develop #3088 +/- ##
==========================================
- Coverage 6.46% 6.45% -0.01%
==========================================
Files 246 246
Lines 12207 12224 +17
==========================================
Hits 789 789
- Misses 11418 11435 +17
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Hi @WildOrangutan!
I'm a big fan of your imageData.getImageUrl method, but not a big fan at all of ImageDescriptor (first remark: it should be private) (second remark: I believe it makes the code complex for no added value).
Please have a look at my comments and tell me what you think.
| } | ||
| } | ||
|
|
||
| class ImageDescriptor { |
There was a problem hiding this comment.
Not a big fan of that class to be honest.
Please remember that if it's standard code that can be reused, it has probably a good chance to land in off-dart instead of Smoothie.
I think it's the case here, so temporarily just code a static method for "get full image url" in Smoothie, that will move to off-dat within a week.
| title: appLocalizations.ingredients, | ||
| buttonText: appLocalizations.ingredients_photo, | ||
| ), | ||
| ProductImageData( | ||
| imageField: ImageField.NUTRITION, | ||
| imageUrl: product.imageNutritionUrl, | ||
| imageDescriptor: ImageDescriptor.fromUrl(product.imageNutritionUrl), |
There was a problem hiding this comment.
This is a typical case where I don't like your ImageDescriptor: that's internally that ProductImageData should deal with it, like a black box. What's the added value of always having to add ImageDescriptor.from(...) in all constructor calls: just do it inside your constructor if you need it.
| buttonText: image.imgid ?? '', | ||
| imageUrl: ImageHelper.buildUrl(widget.product.barcode, image), | ||
| ); | ||
| ProductImageData _getProductImageData(ProductImage image) { |
There was a problem hiding this comment.
You should put that as a factory/constructor in class ProductImageData, shouldn't you?
Indeed, I was confused between |
|
@WildOrangutan You may find interesting new methods in openfoodfacts/openfoodfacts-dart#572 |
|
@monsieurtanuki not sure new method helps me. I'm looking at getImageFilename(). I probably can't use it, since not all |
|
@monsieurtanuki regarding review, I just threw something together, so we could discus it. I reverted most of the changes and added a method to convert url on demand, if needed. I think this is in the lines you want me to do. |
There was a problem hiding this comment.
Hi @WildOrangutan!
I wish we could have used my brand new methods, but you're right, it does not seem relevant :(
The code looks OK, except a big fat bug that I commented... Probably a last minute code cleaning typo.
Not sure how this PR is supposed to fix the issue, as it looks like a good refactoring but I fail to see what will change for the user.
This reverts commit 1a02022.
Maybe API could change to return
In general, it will improve picture quality for user. It will also prevent image quality degradation, when image is edited. Since With current changes, downside is, that user might sometimes not see an image at all, since image for that url might not exist on backend. I would consider this MR a temporary improvement for image quality.
|
That was my understanding of #2960.
That's what I was afraid of: I think the point is to get quickly a degraded picture by default, and only show the full image when 1. you click on it to see its details 2. you click on it to edit it. The full nice picture should be the exception. |
|
"It will also prevent image quality degradation, when image is edited. "
|
The scope of that issue is only to fix image quality for my 2nd screenshoot. The only "bonus" in this MR is downloading "full" image quality for edit screen on my 3rd screenshoot.
The thing is, that "lousy" image from 1st screen are used as arguments and passed to 2nd screen. Since it's possible to obtain less hacky version of image url on 1st screen, without modifying url, I decided to use it. |
|
My point is that it would be confusing for the user to see a lousy picture on one screen, to click on it to see details and to see nothing because the better picture is not downloaded yet (offline or bad connection). The lousy picture should be the fallback, e.g. with FutureBuilder.
That's my only concern here, I do approve the refactoring of this PR.
…________________________________
De : Peter ***@***.***>
Envoyé : samedi 8 octobre 2022 23:11
À : openfoodfacts/smooth-app ***@***.***>
Cc : monsieurtanuki ***@***.***>; Mention ***@***.***>
Objet : Re: [openfoodfacts/smooth-app] refactor: ProductImageData to contain all image links (PR #3088)
That was my understanding of 2960
The scope of that issue is only to fix image quality for my 2nd screenshoot. The only "bonus" in this MR is downloading "full" image quality for edit screen on my 3rd screenshoot.
And that's not what your first screenshot line is about: to me it should be the same "lousy" pictures left and right for "all images"
The thing is, that "lousy" image from 1st screen are used as arguments and passed to 2nd screen. Since it's possible to obtain less hacky version of image url on 1st screen, without modifying url, I decided to use it.
I used "400" quality picture as a middle ground between 1st and 2nd screen.
—
Reply to this email directly, view it on GitHub<#3088 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACYKI37PUIECJHPFWJ4JPOTWCHPRLANCNFSM6AAAAAAQ32RMUU>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
I don't like the idea of brute forcing if image url works and then falling back to lousy image. Because product image is displayed a few times, that may even require it's own widget to handle that. I think it's not a good idea, especially since we in most cases have the information what kind of image qualities are available. We either have to do more work when mapping the data, or later when falling back to lousy image. I'm in favor of mapping data before hand, something like 98a717f. I would go with that concept, and try to polish the code a bit more. |
|
I can't say anything about the code (on my phone atm) but as far as I understand the latest discussion it's about the loading of the larger image. I 100% agree with monsiertarnuki about the loading, it would be better to show the smaller image, but I'm in favor of quickly releasing a fix for higher image size. It's strange to see this and we already got multiple bad reviews about this topic due to users thinking the app is broken, upload the images in such a bad quality. After that we can further improve this as we already have some further work the to do for the whole image update edit flow. My only request is to at least show something while loading (CircularProgressIndicator) |
It's not "then fallback", it's "quick and lousy and available first, then try the full picture". The idea is to display something, even when the internet connection is bad. Like in the old days when jpg pictures were displayed gradually. That said, I have not followed all the issues about image display, and I don't know if users are more complaining
The thing is that the difference between
|
|
I understand what you're trying to say, but due to where potentially non-existent url is used (here) and given that current UX uses download dialog, there is no other way than try to download "full" image first, and if it doesn't exit, download from url where we know it exists ("400" in current code state). I will push some mockup code, to see what I mean. This file is downloaded for the screen my 3rd screenshoot. As I said, I've used same image quality for 1st and 2nd screenshoot. So there is no loading on 2nd screen, because I presume it gets cached on 1st screen. PS: I've used "400" quality 2nd screen. I'm guessing that is ok? |
|
Analyzing smooth-app... info • Use Flutter TODO format: // TODO(username): message, https://URL-to-issue • packages/smooth_app/lib/pages/product/product_image_viewer.dart:147:7 • flutter_style_todos 1 issue found. (ran in 38.3s) |
|
@teolemon thanks, I will resolve it, after we agree if current draft is ok |
I think I understand why we don't understand each other: we're not talking about the same thing. Regarding the "EDIT/CROP" button, it's fair to download the best picture possible, and that's what the issue is about. What I found strange about your screenshots is the improvement of picture quality in the first line between the 2 cols. There the pictures should NOT be downloaded with That being said, if you suggest to dismiss altogether I hope that makes more sense. |
No worries, quality is 400 there, due to reasons above. I'm glad we're getting on same page :) Will update with necessary changes. |
There was a problem hiding this comment.
Hi @WildOrangutan!
That looks good to me, except for a method that should be more explicit - please read my comments.
| ); | ||
| ProductImage? _selectProductImage(Iterable<ProductImage> images) => | ||
| images.firstWhereOrNull( | ||
| (ProductImage image) => image.size == ImageSize.DISPLAY) ?? |
There was a problem hiding this comment.
@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)There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also, what about ORIGINAL, UNKOWN or null image sizes? Isn't it better for image load too slow, other than not load at all?
There was a problem hiding this comment.
@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
400versions 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. "
200is good enough" or "neverfullorunknown").
So let's go with it!
What
Improve product image quality for some pages. For example, when full screen image viewer is open (please see issue description).
My idea was to refactor
ProductImageDatato contain aMapof all product images for every image size, so you can get specific image quality you want, when you need it.I'm submitting this as draft for now, since I'm not completely sure if that is the best approach. Open to ideas. Maybe better backend support would be needed.
Screenshot
Uploading image.png…
Fixes bug(s)