Skip to content

Conversation

@arunjose696
Copy link
Contributor

All callers currently use a zoom context with targetZoom == nativeZoom. This change passes a single integer zoom to AbstractImageProviderWrapper::newImageData, removing the unnecessary zoomContext parameter.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 20, 2025

Test Results

  118 files  ±0    118 suites  ±0   18m 49s ⏱️ + 3m 36s
4 653 tests ±0  4 636 ✅ ±0  17 💤 ±0  0 ❌ ±0 
  338 runs  ±0    334 ✅ ±0   4 💤 ±0  0 ❌ ±0 

Results for commit a58a38d. ± Comparison against base commit 77f9654.

♻️ This comment has been updated with latest results.

@akoch-yatta akoch-yatta force-pushed the arunjose696/532/zoomContextToZoom branch from 51d9bc0 to 766cc96 Compare November 24, 2025 08:33
@laeubi
Copy link
Contributor

laeubi commented Nov 24, 2025

@arunjose696 @akoch-yatta I'd like to make you aware of

where I found the whole concept of passing zoom values (only) both hard to use reliable (e.g. zoom is only integer value while one requires sometimes fractional) and unflexible (one has to compute a suitable zoom level in the "upper layer" to get a matching image while one usually knows the target size quite well.

I therefore like to suggest to rethink the concept of zoom levels really match well for image providers (I think it does not). and not unify this to instead always use a target size what can trivially be derived from a given initial size and zoom level.

Such a TargetSize could then be a record of (double zoom, int width, int height) with the contract that if width + height have values > 0 this is assumed to be the "native size at zoom = 1 -> 100%". That way one can use that to override the "native size" and gets a scaled variant with the given zoom level (e.g. it might later be zoomed to 150% = zoom 1.5), runding would of course still happen but it will be as much exact as possible.

FYI @HeikoKlare

@arunjose696
Copy link
Contributor Author

arunjose696 commented Nov 25, 2025

@laeubi I’m not sure if PR 3431
has any effect on this change this one is just a minor refactoring.

I’m also slightly confused by the approach in your proposal: having default values for height and width in the TargetSize record feels a bit counterintuitive. I would expect this to be seperated, or wouldnt the callers requesting image at a target size need to know about the default size of the image?

That said, I think these two PRs are unrelated and can be treated separately and if we need to have a target size along with zoom that would be a different proposal. I have also breifly reviewed the open PR

All callers currently use a zoom context with targetZoom == nativeZoom.
This change passes a single integer zoom to AbstractImageProviderWrapper::newImageData,
removing the unnecessary zoomContext parameter.
@akoch-yatta akoch-yatta force-pushed the arunjose696/532/zoomContextToZoom branch from 766cc96 to a58a38d Compare December 1, 2025 09:11
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

The change is a pure refactoring/simplification and fine as such. I have two minor comments.

ImageData newImageData(ZoomContext zoomContext) {
int targetZoom = zoomContext.targetZoom();
ImageData newImageData(int zoom) {
int targetZoom = zoom;
Copy link
Contributor

Choose a reason for hiding this comment

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

targetZoom could be completely removed here

ImageData newImageData(int zoom) {
Function<Integer, ImageData> imageDataRetrieval = zoomToRetrieve -> {
ImageHandle handle = initializeHandleFromSource(zoomContext);
ImageHandle handle = initializeHandleFromSource(new ZoomContext(zoom));
Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of consistency, shouldn't this anyway be ... ?

Suggested change
ImageHandle handle = initializeHandleFromSource(new ZoomContext(zoom));
ImageHandle handle = initializeHandleFromSource(new ZoomContext(zoomToRetrieve));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Modify AbstractImageProviderWrapper::newImageData to just take in zoom instead of zoom context

3 participants