HARP-12687: User Images are removed from cache when theme is changed #1931
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1931 +/- ##
==========================================
+ Coverage 65.25% 65.26% +0.01%
==========================================
Files 293 293
Lines 26329 26331 +2
Branches 5951 5951
==========================================
+ Hits 17181 17185 +4
+ Misses 9148 9146 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks good to me, but as I understand the ticket, but does it solve the issue? Or maybe there is a follow up planned?
Wouldn´t the MapView.userImageCache and the MapView.imageCache register Images referencing the same MapView, and therefore if MapView.imageCache is cleared it would still clear the userImageCache, as described in the ticket?
That's right. This change does not solve the issue. |
|
That is right, my bad. I caught hung up in fixing the issue when removing shared images (which still happens when the theme is changed). |
StefanDachwitz
force-pushed
the
HARP-12687
branch
from
8138218
to
5d07b49
Compare
| @@ -25,30 +24,31 @@ declare function createImageBitmap( | |||
| ): Promise<ImageBitmap>; | |||
|
|
|||
| /** | |||
| * Combines an {@link ImageItem} with a list of {@link MapView}s that reference it. | |||
| * Combines an {@link ImageItem} with a list of owners that reference it. Although an owner can be | |||
| * any type of object, it should be of type {@link MapViewImageCache}. | |||
There was a problem hiding this comment.
is there a reason to mention "it should be of type MapViewImageCache", it doesn't really matter?
Also, please mention that the type is irrelevant, because we are just refcounting, hence any is a valid type.
| url: string, | ||
| imageData?: ImageData | ImageBitmap, | ||
| htmlElement?: HTMLImageElement | HTMLCanvasElement | ||
| ): ImageItem { | ||
| let imageCacheItem = this.findImageCacheItem(url); | ||
| if (imageCacheItem !== undefined) { | ||
| if (mapView !== undefined && !imageCacheItem.mapViews.includes(mapView)) { | ||
| imageCacheItem.mapViews.push(mapView); | ||
| if (owner !== undefined && !imageCacheItem.owners.includes(owner)) { |
There was a problem hiding this comment.
do you need the undefined check? The owner is not optional.
| @@ -120,7 +121,7 @@ export class ImageCache { | |||
| htmlElement: htmlElement, | |||
| loaded: false | |||
| }, | |||
| mapViews | |||
| owners: owners | |||
There was a problem hiding this comment.
| owners: owners | |
| owners: [owner] |
There was a problem hiding this comment.
applied your suggestion
|
Reworked the PR to use a proper interface for owners. |
* remove images from cache only if not used in any other MapView Signed-off-by: stefan.dachwitz <stefan.dachwitz@here.com>
* made MapViewImageCache (instead of MapView) the owner of an ImageCacheItem Signed-off-by: stefan.dachwitz <stefan.dachwitz@here.com>
Signed-off-by: stefan.dachwitz <stefan.dachwitz@here.com>
StefanDachwitz
force-pushed
the
HARP-12687
branch
from
2e92e56
to
abaa399
Compare
|
retest this please |
Signed-off-by: stefan.dachwitz stefan.dachwitz@here.com
Thank you for contributing to harp.gl!
Before requesting a pull request, please remember to check the following documents:
If you are adding new functionality we would highly appreciate if you can describe what is the capability you are adding and even better if you can add some examples. Please also remember to add tests for it.
CI Check
Our bots will check whether your PR can be directly integrated into the mainline. We have some internal integration tests running on the background, our bots will inform you of the next steps and someone from our team will take a look and help if needed!
And please do not forget to sign-off your commit! You can read more about DCO here. But, in short, you just need to use
git commit -sor append--signoffwhen you are committing to the repo.Happy contributing!