zathras / jovial_svg

Flutter library for robust, efficient rendering of SVG static images
BSD 3-Clause "New" or "Revised" License
115 stars 21 forks source link

Cache keys include irrelevant fields in hashCode/equals #103

Closed SrdjanCBS closed 4 months ago

SrdjanCBS commented 4 months ago

Issue Description:

When using the ScalableImageWidget components to load and display images, there is noticeable flashing during re-renders even when the images are cached. This disrupts the user experience and indicates that the caching mechanism is not functioning as expected in this context.

Expected Behavior:

Images should be displayed smoothly from the cache without any visible flashing or delay.

Actual Behavior:

There is a visible flash when the images re-render, suggesting they are being reloaded rather than displayed from the cache.

Sample Code:

`class RoundImage extends StatelessWidget { const RoundImage({ Key? key }) : super(key: key);

@override Widget build(BuildContext context) { return ScalableImageWidget.fromSISource( onError: () { return Image.network( '$image.png', fit: boxFit, ); }, si: ScalableImageSource.fromSvgHttpUrl( Uri.parse('$image.svg'), warnF: () {}, ), cache: ScalableImageCache(size: 100), ); } }`

I am using this RoundImage widget everywhere in app.

https://github.com/user-attachments/assets/75d05950-698c-4d93-bcef-444a763a8cb2

zathras commented 4 months ago

How many different images are there in the cache? You'd also want to structure the code to make sure you're not falling back on the png image due to an error; the code you've shown doesn't do that. This is quite possibly a bug in the application code, but since it's incomplete, I don't have enough information to tell.

To proceed, I'd need a reproducible test case -- a small program with full source that demonstrates the problem.

SrdjanCBS commented 4 months ago

@zathras Here is code example: https://github.com/SrdjanCBS/jovial_svg_test

I created sample app showing problem that I am occurring. Maybe there is a bug in the application but I wanted to check with you. For me, The most important feature from this package is onError callback which I couldn't find In any other package. Sometimes svg that I am fetching don't exist but png does so this is the reason why I return regular image on error and it works perfectly.

https://github.com/user-attachments/assets/052a8407-9551-4abc-aded-d7ad78459132

zathras commented 4 months ago

OK, but for debugging purposes, you should show something else on error. That way, you can narrow down what might be happening. That's why library maintainers generally ask for a minimal sample that reproduces the problem. The ScalableImageCache can't cache your PNG images; if you show something simpler for debugging purposes, that narrows down the potential problems. A blank red widget would do.

Oh, wait. Just looking at your code, you're passing in a new cache every time you build. If you create a new cache every time, it will be empty, so of course you'll re-load the asset every time. See the cache example program, as referenced in the ScalableImageCache documentation.

SrdjanCBS commented 4 months ago

@zathras I've taken a close look at your caching example and used it to simplify my app down to the basics, ensuring the code closely mirrors yours. I've removed the onError callback as well. However, the images are still flashing. In your caching example, you're fetching SVG images and displaying them in a list view, but there are no state changes or re-rendering involved.

https://github.com/user-attachments/assets/b3558192-e713-41d9-8f2d-cd96ec0f0a8e

zathras commented 4 months ago

Without seeing the code, there's not much I can say. Did you fix the part where you were creating a new cache in the build method?

SrdjanCBS commented 4 months ago

https://github.com/SrdjanCBS/jovial_svg_test

zathras commented 4 months ago

Oh, look at that! You're right, there was a bug in jovial_svg. (There was also a bug in your app - making a new cache every time - which obscured the jovial_svg bug).

As a workaround, you can probably just not specify warnF, or maybe make it a static method. I'll also be publishing as version 1.1.22-rc.1 pretty soon - please let me know if that fixes the issues.

Thanks!

SrdjanCBS commented 4 months ago

@zathras Yes, when i remove warnF it works fine even without caching. Thanks!

zathras commented 4 months ago

Published in 1.1.22