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 1 commit
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
51 changes: 10 additions & 41 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,7 +11,7 @@ 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,
Expand All @@ -22,33 +22,6 @@ class ProductImageWidget extends StatefulWidget {
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;

@override
Widget build(BuildContext context) {
final SmoothColorsThemeExtension colors =
Expand All @@ -57,20 +30,20 @@ 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,
),
),
rounded: false,
);
final DateTime? uploaded = widget.productImage.uploaded;
final DateTime? uploaded = productImage.uploaded;
if (uploaded == null) {
return image;
}
Expand All @@ -85,7 +58,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 +81,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