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

fix: Fix performances on the photos gallery #5447

Merged
merged 3 commits into from
Jun 28, 2024
Merged

fix: Fix performances on the photos gallery #5447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27cd2d8
Improve performances for the photos gallery
g123k Jun 27, 2024
88b07ae
Allow to pass an `ImageSize` to `ProductImageWidget`
g123k Jun 27, 2024
fdbbb34
Update packages/smooth_app/lib/pages/image/product_image_gallery_othe…
g123k Jun 27, 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
38 changes: 29 additions & 9 deletions packages/smooth_app/lib/generic_lib/widgets/images/smooth_image.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,16 @@ class SmoothImage extends StatelessWidget {
this.fit = BoxFit.cover,
this.rounded = true,
this.heroTag,
});
this.cacheWidth,
this.cacheHeight,
}) : assert(
cacheWidth == null || imageProvider is NetworkImage,
'cacheWidth requires a NetworkImage',
),
assert(
cacheHeight == null || imageProvider is NetworkImage,
'cacheHeight requires a NetworkImage',
);

final ImageProvider? imageProvider;
final double? height;
Expand All @@ -28,17 +37,28 @@ class SmoothImage extends StatelessWidget {
final BoxFit fit;
final String? heroTag;
final bool rounded;
final int? cacheWidth;
final int? cacheHeight;
Comment on lines +40 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

Those cache parameters may be helpful regarding memory, but not network latency.
Please consider implementing my suggestion about ImageSize.DISPLAY. That will dramatically help for memory too!

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.
I think, I will also add all variants of ImageSize

Copy link
Contributor

Choose a reason for hiding this comment

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

Good. Btw I don't think all variants are always implemented; I would focus on 400 and FULL only.


@override
Widget build(BuildContext context) {
Widget child = imageProvider == null
? const PictureNotFound()
: Image(
image: imageProvider!,
fit: fit,
loadingBuilder: _loadingBuilder,
errorBuilder: _errorBuilder,
);
Widget child = switch (imageProvider) {
NetworkImage(url: final String url) => Image.network(
url,
fit: fit,
loadingBuilder: _loadingBuilder,
errorBuilder: _errorBuilder,
cacheWidth: cacheWidth,
cacheHeight: cacheHeight,
),
ImageProvider<Object>() => Image(
image: imageProvider!,
fit: fit,
loadingBuilder: _loadingBuilder,
errorBuilder: _errorBuilder,
),
_ => const PictureNotFound(),
};

if (heroTag != null) {
child = Hero(tag: heroTag!, child: child);
Expand Down
12 changes: 12 additions & 0 deletions packages/smooth_app/lib/pages/image/product_image_gallery_other_view.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'package:collection/collection.dart';
import 'package:flutter/material.dart';
import 'package:flutter_gen/gen_l10n/app_localizations.dart';
import 'package:openfoodfacts/openfoodfacts.dart';
Expand Down Expand Up @@ -108,6 +109,8 @@ class _RawGridGallery extends StatelessWidget {
@override
Widget build(BuildContext context) {
final double squareSize = _getSquareSize(context);
final ImageSize? imageSize = _computeImageSize(squareSize);

return SliverGrid(
gridDelegate: const SliverGridDelegateWithFixedCrossAxisCount(
crossAxisCount: _columns,
Expand Down Expand Up @@ -137,6 +140,7 @@ class _RawGridGallery extends StatelessWidget {
productImage: productImage,
barcode: product.barcode!,
squareSize: squareSize,
imageSize: imageSize,
),
),
);
Expand All @@ -146,4 +150,12 @@ class _RawGridGallery extends StatelessWidget {
),
);
}

ImageSize? _computeImageSize(double squareSize) => <ImageSize>[
ImageSize.THUMB,
ImageSize.SMALL,
Comment on lines +155 to +156
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ImageSize.THUMB,
ImageSize.SMALL,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's OK with THUMB and SMALL, I don't really understand why you want to ditch them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I want to ditch them because I have experience in images and I remember that not all values are always populated, only 400 and FULL.
Just as an example, I've tried the latest barcode I used (https://fr.openfoodfacts.org/api/v3/product/3661344961326?fields=images):

{
  "code": "3661344961326",
  "errors": [],
  "product": {
    "images": {
      "1": {
        "sizes": {
          "100": {
            "h": 100,
            "w": 75
          },
          "400": {
            "h": 400,
            "w": 300
          },
          "full": {
            "h": 3264,
            "w": 2448
          }
        },
        "uploaded_t": 1428607891,
        "uploader": "openfoodfacts-contributors"
      },
      "12": {
        "sizes": {
          "100": {
            "h": 100,
            "w": 99
          },
          "400": {
            "h": 400,
            "w": 396
          },
          "full": {
            "h": 2475,
            "w": 2448
          }
        },
        "uploaded_t": 1550253015,
        "uploader": "moon-rabbit"
      },

And so on.
It looks like "raw" images only have 100, 400 and full, never 200.

That's why I recommend 400 or FULL:

  • 200 doesn't seem to exist for "raw" images, at least on a regular basis
  • it's highly unlikely that user screens are less than 3*100 wide
  • if we summon a 200 image that doesn't exist, that will look like unknown image
  • the code would be much easier to maintain in a simple 400 or FULL choice
  • the added value of the current code is "optimized image versus full image" (regardless of 100, 200 or 400) - and the added value of 100 versus 200 or 400 is probably less important

ImageSize.DISPLAY
].firstWhereOrNull(
(ImageSize element) => squareSize <= int.parse(element.number),
);
}
54 changes: 14 additions & 40 deletions packages/smooth_app/lib/pages/image/product_image_widget.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,43 +11,20 @@ import 'package:smooth_app/resources/app_icons.dart';
import 'package:smooth_app/themes/smooth_theme_colors.dart';

/// Displays a product image thumbnail with the upload date on top.
class ProductImageWidget extends StatefulWidget {
class ProductImageWidget extends StatelessWidget {
const ProductImageWidget({
required this.productImage,
required this.barcode,
required this.squareSize,
this.imageSize,
});

final ProductImage productImage;
final String barcode;
final double squareSize;

@override
State<ProductImageWidget> createState() => _ProductImageWidgetState();
}

class _ProductImageWidgetState extends State<ProductImageWidget> {
@override
void initState() {
super.initState();
_loadImagePalette();
}

Future<void> _loadImagePalette() async {
final ColorScheme palette = await ColorScheme.fromImageProvider(
provider: NetworkImage(widget.productImage.getUrl(
widget.barcode,
uriHelper: ProductQuery.uriProductHelper,
)));

setState(() {
backgroundColor = palette.primaryContainer;
darkBackground = backgroundColor!.computeLuminance() < 0.5;
});
}

Color? backgroundColor;
bool? darkBackground;
/// Allows to fetch the optimized version of the image
final ImageSize? imageSize;

@override
Widget build(BuildContext context) {
Expand All @@ -57,20 +34,21 @@ class _ProductImageWidgetState extends State<ProductImageWidget> {
final DateFormat dateFormat =
DateFormat.yMd(ProductQuery.getLanguage().offTag);

darkBackground = darkBackground ?? true;

final Widget image = SmoothImage(
width: widget.squareSize,
height: widget.squareSize,
cacheHeight:
Copy link
Contributor

Choose a reason for hiding this comment

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

No cacheWidth?

It's a bit strange to say "you can force ImageSize if you want, but regardless we will force a cacheHeight even if you don't want to and there's nothing you can do about 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.

Here we have a square, but the server's photo may have a different ratio.
If I say, cacheWidth == cacheHeight, it means the server picture is a square.
That's why, I only provide one of the argument

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the square effect.

I don't know how relevant this cache option would be, performance-wise.
My assumption is that the best performance improvement is with smaller images (400).

I also fear that this cache option entail side-effects: what if I download a full image with that cache option for a thumbnail, and then display the full image on a full screen after a tap? From the docs, I assume that the cached image will be stored as resized, and then reused, which would render a pixelized image.

At least, you could add a comment about why cacheHeight but not cacheWidth.

(squareSize * MediaQuery.devicePixelRatioOf(context)).toInt(),
width: squareSize,
height: squareSize,
imageProvider: NetworkImage(
widget.productImage.getUrl(
widget.barcode,
productImage.getUrl(
barcode,
uriHelper: ProductQuery.uriProductHelper,
imageSize: imageSize,
),
),
rounded: false,
);
final DateTime? uploaded = widget.productImage.uploaded;
final DateTime? uploaded = productImage.uploaded;
if (uploaded == null) {
return image;
}
Expand All @@ -85,7 +63,7 @@ class _ProductImageWidgetState extends State<ProductImageWidget> {
button: true,
child: SmoothCard(
padding: EdgeInsets.zero,
color: backgroundColor ?? colors.primaryBlack,
color: colors.primaryBlack,
borderRadius: ANGULAR_BORDER_RADIUS,
margin: EdgeInsets.zero,
child: ClipRRect(
Expand All @@ -108,11 +86,7 @@ class _ProductImageWidgetState extends State<ProductImageWidget> {
child: AutoSizeText(
date,
maxLines: 1,
style: TextStyle(
color: darkBackground!
? Colors.white
: colors.primaryDark,
),
style: const TextStyle(color: Colors.white),
),
),
if (expired)
Expand Down
Loading