feat: image list gallery #2724

VaiTon · 2022-08-03T19:29:47Z

In V1 (at least for android) we had a "Image gallery" page which would display the list of every image related to the product.

Here we have the carousel that gets activated whenever you click on the images (or click on edit images in the edit view) but it's not really a listing of every image related to the product.

That's why, as suggested by @teolemon, with this PR I'm implementing a product image gallery view as a list of clickable items. When a tile is clicked the corresponding image is opened (if existing) to be viewed/edited, or taken.

Below a video representing the flow:

2022-08-05.00-54-25.mp4

Updated screenshot

codecov-commenter · 2022-08-03T19:47:50Z

Codecov Report

Merging #2724 (0b580a3) into develop (2ea0da3) will decrease coverage by 1.82%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2724      +/-   ##
==========================================
- Coverage     8.86%   7.04%   -1.83%     
==========================================
  Files          161     223      +62     
  Lines         6623   10679    +4056     
==========================================
+ Hits           587     752     +165     
- Misses        6036    9927    +3891

Impacted Files	Coverage Δ
...kages/smooth_app/lib/widgets/attribute_button.dart	`0.00% <0.00%> (-92.00%)`	⬇️
...s/smooth_app/lib/data_models/user_preferences.dart	`8.73% <0.00%> (-23.57%)`	⬇️
packages/smooth_app/lib/themes/smooth_theme.dart	`62.26% <0.00%> (-20.72%)`	⬇️
...p/lib/generic_lib/dialogs/smooth_alert_dialog.dart	`15.11% <0.00%> (-19.10%)`	⬇️
...mooth_app/lib/data_models/product_preferences.dart	`21.68% <0.00%> (-9.75%)`	⬇️
packages/smooth_app/lib/main.dart	`14.65% <0.00%> (-3.24%)`	⬇️
.../smooth_app/lib/pages/onboarding/welcome_page.dart	`0.00% <0.00%> (-3.13%)`	⬇️
.../smooth_app/lib/pages/onboarding/scan_example.dart	`0.00% <0.00%> (-2.28%)`	⬇️
...ackages/smooth_app/lib/pages/scan/scan_header.dart	`2.50% <0.00%> (-2.27%)`	⬇️
...p/lib/pages/onboarding/consent_analytics_page.dart	`0.00% <0.00%> (-1.57%)`	⬇️
... and 244 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

VaiTon · 2022-08-04T14:29:34Z

@monsieurtanuki @M123-dev @g123k if you could give me an initial code review it would be great!

monsieurtanuki · 2022-08-04T17:10:11Z

@VaiTon For the record it's a "draft": not something reviewers spend time on spontaneously. Unless instructed to :)

monsieurtanuki · 2022-08-04T17:14:17Z

@VaiTon I don't mean to be rude but you don't provide any explanation on the PR and any /// comment in the code. We don't know what you do and why you do it. That kind of information can help reviewers.

VaiTon · 2022-08-04T22:40:19Z

@VaiTon I don't mean to be rude but you don't provide any explanation on the PR and any /// comment in the code. We don't know what you do and why you do it. That kind of information can help reviewers.

Sorry! I really thought I put something in the body of the PR while I didn't 😆. For the Draft, I did that on purpose but I'd like your review code wise to continue this path or not.

edit @monsieurtanuki added an explanation and a video!

monsieurtanuki

Hi @VaiTon!

Nothing that shocks me in your code, except the ignore_for_file: cast_nullable_to_non_nullable that is definitely NOT a good idea and needs to be removed immediately.

Other suggestions:

don't hesitate to put /// comments
don't hesitate, instead of lousy Widget _buildBlablabla methods, to create a new widget in a new specific class (with its own /// comments, and maybe in its own specific file) - it's better for clarity and flutter performances

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

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

M123-dev · 2022-08-05T16:54:38Z

Havn't had a look at the code, but this is definitely great to have, AFAIK the images are the most important part of the product, so we should make it easy to edit and send them.

We can use this and build upon.

Some related issues:

Improve the Photo Manager using new features from openfoodfacts-dart #766
Add Server-side image cropping and rotation in the Photo manager (expanded view accessible from the photo carousel) #2484
Add image rotation (lower priority, server side, same view as cropping) in the expanded view accessible from the carousel #2429
Add image unselect in the expanded view accessible from the carousel #2427
Leverage support for the Image refresh API to make a custom Call To Action to take a new photo #2426
Add image selection from server in the Photo Manager (the expanded view accessible from the carousel) #2424
Reintroduce "All photos" manager from V1 as a new section in edition #2351

VaiTon · 2022-08-05T19:43:59Z

The main issue I found while coding this is that at the moment we only have like 4 categories displayed instead of every image.

For example the Nutella has lots of images on the website but on the app only 4 are displayed.

M123-dev · 2022-08-05T20:32:09Z

Do we need to show more then that

monsieurtanuki · 2022-08-06T08:34:06Z

I don't know what we're supposed to display in Smoothie, but this is the data we get from the server (https://world.openfoodfacts.org/cgi/search.pl?code=3017620425035&json=1&search_terms=):

"image_front_small_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/front_en.381.200.jpg",
"image_front_thumb_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/front_en.381.100.jpg",
"image_front_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/front_en.381.400.jpg",
"image_ingredients_small_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/ingredients_en.379.200.jpg",
"image_ingredients_thumb_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/ingredients_en.379.100.jpg",
"image_ingredients_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/ingredients_en.379.400.jpg",
"image_nutrition_small_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/nutrition_en.368.200.jpg",
"image_nutrition_thumb_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/nutrition_en.368.100.jpg",
"image_nutrition_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/nutrition_en.368.400.jpg",
"image_packaging_small_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/packaging_en.369.200.jpg",
"image_packaging_thumb_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/packaging_en.369.100.jpg",
"image_packaging_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/packaging_en.369.400.jpg",
"image_small_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/front_en.381.200.jpg",
"image_thumb_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/front_en.381.100.jpg",
"image_url": "https://images.openfoodfacts.org/images/products/301/762/042/5035/front_en.381.400.jpg",

teolemon · 2022-08-06T09:57:42Z

Part of this is Reintroduce "All photos" manager from V1 as a new section in edition #2351. The server is sending the IDs of all stored images.

VaiTon · 2022-08-06T10:15:18Z

@monsieurtanuki the query that we do in the Android app is:

/**
 * Returns images for the current product
 *
 * @param barcode barcode for the current product
 */
@GET("$API_P/product/{barcode}.json?fields=images")
suspend fun getProductImages(@Path("barcode") barcode: String): ObjectNode

monsieurtanuki · 2022-08-06T11:57:27Z

@VaiTon Add ProductField.images to ProductQuery.fields, and find the results in Product.images.

VaiTon · 2022-08-06T14:46:39Z

@VaiTon Add ProductField.images to ProductQuery.fields, and find the results in Product.images.

Where should I do this? The idea was to query for all the images only in the gallery view

…ta` to better reflect its work

monsieurtanuki · 2022-08-06T15:04:06Z

Where should I do this? The idea was to query for all the images only in the gallery view

Either you do that always for all products, and add ProductField.images to ProductQuery.fields.

Or you want specifically to extract those images only if the user goes to a specific page: then you have to asyncly download only the ProductField.images for that product. Something similar to what is coded in BarcodeProductQuery, like:

  Future<List<ProductImage>?> getFetchedProduct() async {
    final ProductQueryConfiguration configuration = ProductQueryConfiguration(
      barcode,
      fields: [ProductQuery.IMAGES],
      language: ProductQuery.getLanguage(),
      country: ProductQuery.getCountry(),
    );

    final ProductResult result;
    try {
      result = await OpenFoodAPIClient.getProduct(configuration);
    } catch (e) {
      return null;
    }

    if (result.status == 1) {
      final Product? product = result.product;
      if (product != null) {
        return product.images;
      }
    }
    return null;
  }

But then you probably have to cache the results somehow...

VaiTon · 2022-08-06T15:06:36Z

@monsieurtanuki thanks! I think that we maybe could do without caching for now, as editing images requires an internet connection to download them (as far as I understand)

M123-dev · 2022-08-07T09:04:06Z

/**
 * Returns images for the current product
 *
 * @param barcode barcode for the current product
 */
@GET("$API_P/product/{barcode}.json?fields=images")
suspend fun getProductImages(@Path("barcode") barcode: String): ObjectNode

Ohh luckily I don't know kotlin if you have to suspend fun in it 😆

VaiTon · 2022-08-07T18:50:31Z

Now it should display every image. I'm not understanding why they're not opening in full screen anymore though..

…dart Co-authored-by: monsieurtanuki <fabrice_fontaine@hotmail.com>

# Conflicts: # packages/smooth_app/lib/pages/product/edit_product_page.dart

VaiTon · 2022-08-16T15:14:08Z

Updated screenshot

monsieurtanuki

Hi @VaiTon: the SmoothBackButton is cool, but I have other comments too.

packages/smooth_app/lib/generic_lib/widgets/smooth_list_tile_card.dart

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

monsieurtanuki · 2022-08-16T15:37:01Z

packages/smooth_app/lib/generic_lib/widgets/smooth_list_tile_card.dart

+import 'package:smooth_app/generic_lib/widgets/smooth_product_image_container.dart';
+import 'package:smooth_app/themes/constant_icons.dart';
+
+class SmoothListTileCard extends StatelessWidget {


The big misunderstanding is that I never imagined that you would display an image inside a "normal" ListTile. I expected something similar to the website: tons of pictures in a grid.

I don't know if there were explicit expectations regarding UX/UI, but I must say I'm not a big fan of:

all the waste space with no added value (the "title")

the cropped image

the image ratio

We should start from there: how is it supposed to look? @teolemon Are you OK with the latest screenshots?

…_card.dart`

Co-authored-by: monsieurtanuki <fabrice_fontaine@hotmail.com>

teolemon · 2022-08-16T16:15:10Z

thanks 👍
I was indeed expecting a grid of squares for unselected images, like in v1 (for extreme cases, and also because you shouldn't be able to do all the same operations on an unselected image as on a selected one, eg client or server side cropping, rotation, unless you're explicitely mapping them to a field first (and server side operations would be preferable)

VaiTon · 2022-08-16T16:16:59Z

thanks +1 I was indeed expecting a grid of squares for unselected images, like in v1 (for extreme cases, and also because you shouldn't be able to do all the same operations on an unselected image as on a selected one, eg client or server side cropping, rotation, unless you're explicitely mapping them to a field first (and server side operations would be preferable)

So we can keep the list for the selected images and a grid for the unselected images? Even keeping a list we would be able to make some actions doable only for selected images

monsieurtanuki · 2022-08-16T16:26:52Z

I don't know the V1, but from what I see in the website it's a bit by chapter: front, ingredients, ...

Let's imagine what we should display for each chapter (selected image with special power, and all the images).

Something like that:

FRONT
selected:
 000000
 000000
 000000

select another image:
 123456
 ABCDEF
 GHIJKL

Take another picture

Btw @VaiTon I would happily (not happily, but seriously) review your code once we agree on the display and your generated screenshots.

teolemon · 2022-08-16T16:29:49Z

@VaiTon @monsieurtanuki here's what I have in Figma

VaiTon · 2022-08-16T16:29:50Z

@monsieurtanuki would that be in a single page? Because unselected images do not have a field so we should display every unselected image for every field

VaiTon · 2022-08-16T16:31:14Z

@teolemon which is what we have on android.

I don't think that's really what we want because it does not separate the selected images from the unselected ones.

teolemon · 2022-08-16T16:32:52Z

So we could use that in 2 places:

one as a server side photo picker (where it makes sense)
and as a variation below the list of selected images, that could stay tiles, like it currently is.

monsieurtanuki · 2022-08-16T16:40:16Z

Personally I had no expectations, except a grid, so I'm not really the person to ask.
@VaiTon You seem to be right regarding "unknown" pictures: on the website there are tons of pictures in a grid, and the same ton for each "chapter". In French we say "vrac", which is a dutch word that sounds perfect to say that pictures are just put somewhere.

VaiTon · 2022-08-16T17:06:44Z

@teolemon what about

VaiTon · 2022-08-16T22:53:49Z

@openfoodfacts/smooth-app any comments? Otherwise I'm implementing this one in a different PR. (50+ comments on this one)

monsieurtanuki · 2022-08-17T07:39:22Z

@VaiTon Your latest screenshot looks good, except that I would not display images with different aspect ratio for "selected" and "unselected" images. As in your screenshot you put 4 "unselected" images in a row, I guess you could put 2 "selected" images in a row with the same aspect ratio.
If it's more convenient for you to create a new PR, please do. And don't forget to display screenshots first, so that we can assess the UI/UX before starting reviewing the code itself.

github-actions bot assigned VaiTon Aug 3, 2022

feat: image list gallery

d748bc6

VaiTon force-pushed the feat/image_gallery branch from 523d7e4 to d748bc6 Compare August 3, 2022 19:31

feat: added arrows and styling to tiles

27e780b

Merge branch 'develop' into feat/image_gallery

be68b27

monsieurtanuki reviewed Aug 5, 2022

View reviewed changes

VaiTon added 3 commits August 5, 2022 13:49

refactor: extract SmoothListTileCard widget

50f1e1d

docs: add documentation to product_image_gallery_view.dart

7b70d61

chore: cleaned code

3e3968c

refactor: rename getAllProductImagesData to `getProductMainImagesDa…

5d53934

…ta` to better reflect its work

VaiTon added 2 commits August 6, 2022 17:06

Merge branch 'develop' into feat/image_gallery

8339953

feat: display every image in gallery view

86af408

Merge branch 'develop' into feat/image_gallery

ae461e4

VaiTon and others added 3 commits August 16, 2022 16:22

Update packages/smooth_app/lib/generic_lib/widgets/smooth_image_list.…

dfcd7b0

…dart Co-authored-by: monsieurtanuki <fabrice_fontaine@hotmail.com>

refactor: extract SmoothBackButton

862241a

refactor: extract methods and add docs in product_image_viewer.dart

efa58c2

github-actions bot removed the PR: needs rebase label Aug 16, 2022

Merge branch 'develop' into feat/image_gallery

dbcd334

# Conflicts: # packages/smooth_app/lib/pages/product/edit_product_page.dart

VaiTon force-pushed the feat/image_gallery branch from ad2384f to dbcd334 Compare August 16, 2022 15:09

monsieurtanuki reviewed Aug 16, 2022

View reviewed changes

VaiTon and others added 4 commits August 16, 2022 17:47

refactor: use _ListTileItem in edit_product_page.dart

0b580a3

refactor: allow better reviews

c21aafc

fix: use relative size instead of arbitrary ones in `smooth_list_tile…

a57a485

…_card.dart`

fix: use black for product_image_viewer.dart AppBar

7a25112

Co-authored-by: monsieurtanuki <fabrice_fontaine@hotmail.com>

VaiTon requested a review from monsieurtanuki August 16, 2022 16:10

fix: use Flutter TODO format

d44439e

teolemon added 🖼️ Photos - Photo manager ✨ enhancement New feature or request labels Aug 17, 2022

VaiTon closed this Aug 17, 2022

teolemon mentioned this pull request Aug 23, 2022

feat: Add a gallery of all the images present on the server (v2) #2801

Merged

monsieurtanuki mentioned this pull request Jan 26, 2023

Add image selection from server in the Photo Manager (the expanded view accessible from the carousel) #2424

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: image list gallery #2724

feat: image list gallery #2724

VaiTon commented Aug 3, 2022 •

edited

Loading

codecov-commenter commented Aug 3, 2022 •

edited

Loading

VaiTon commented Aug 4, 2022

monsieurtanuki commented Aug 4, 2022

monsieurtanuki commented Aug 4, 2022

VaiTon commented Aug 4, 2022 •

edited

Loading

monsieurtanuki left a comment

M123-dev commented Aug 5, 2022 •

edited by teolemon

Loading

VaiTon commented Aug 5, 2022

M123-dev commented Aug 5, 2022

monsieurtanuki commented Aug 6, 2022

teolemon commented Aug 6, 2022

VaiTon commented Aug 6, 2022

monsieurtanuki commented Aug 6, 2022

VaiTon commented Aug 6, 2022

monsieurtanuki commented Aug 6, 2022

VaiTon commented Aug 6, 2022

M123-dev commented Aug 7, 2022

VaiTon commented Aug 7, 2022

VaiTon commented Aug 16, 2022

monsieurtanuki left a comment

monsieurtanuki Aug 16, 2022

teolemon commented Aug 16, 2022

VaiTon commented Aug 16, 2022

monsieurtanuki commented Aug 16, 2022

teolemon commented Aug 16, 2022

VaiTon commented Aug 16, 2022

VaiTon commented Aug 16, 2022

teolemon commented Aug 16, 2022

monsieurtanuki commented Aug 16, 2022

VaiTon commented Aug 16, 2022

VaiTon commented Aug 16, 2022

monsieurtanuki commented Aug 17, 2022

feat: image list gallery #2724

feat: image list gallery #2724

Conversation

VaiTon commented Aug 3, 2022 • edited Loading

Updated screenshot

codecov-commenter commented Aug 3, 2022 • edited Loading

Codecov Report

VaiTon commented Aug 4, 2022

monsieurtanuki commented Aug 4, 2022

monsieurtanuki commented Aug 4, 2022

VaiTon commented Aug 4, 2022 • edited Loading

monsieurtanuki left a comment

Choose a reason for hiding this comment

M123-dev commented Aug 5, 2022 • edited by teolemon Loading

VaiTon commented Aug 5, 2022

M123-dev commented Aug 5, 2022

monsieurtanuki commented Aug 6, 2022

teolemon commented Aug 6, 2022

VaiTon commented Aug 6, 2022

monsieurtanuki commented Aug 6, 2022

VaiTon commented Aug 6, 2022

monsieurtanuki commented Aug 6, 2022

VaiTon commented Aug 6, 2022

M123-dev commented Aug 7, 2022

VaiTon commented Aug 7, 2022

VaiTon commented Aug 16, 2022

Updated screenshot

monsieurtanuki left a comment

Choose a reason for hiding this comment

monsieurtanuki Aug 16, 2022

Choose a reason for hiding this comment

teolemon commented Aug 16, 2022

VaiTon commented Aug 16, 2022

monsieurtanuki commented Aug 16, 2022

teolemon commented Aug 16, 2022

VaiTon commented Aug 16, 2022

VaiTon commented Aug 16, 2022

teolemon commented Aug 16, 2022

monsieurtanuki commented Aug 16, 2022

VaiTon commented Aug 16, 2022

VaiTon commented Aug 16, 2022

monsieurtanuki commented Aug 17, 2022

VaiTon commented Aug 3, 2022 •

edited

Loading

codecov-commenter commented Aug 3, 2022 •

edited

Loading

VaiTon commented Aug 4, 2022 •

edited

Loading

M123-dev commented Aug 5, 2022 •

edited by teolemon

Loading