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.

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

Obtain ImageData for any requested zoom value, not only autoscaled values #1713

Merged

Conversation

amartya4256
Copy link
Contributor

This contribution helps retrieve the imageData for the requested zoom by searching for the zoom in the zoomToHandleMap followed by (if not available) scaling the handle at the zoom nearest to it while getting rid of only searching among the autoscaled zoom values.

Contributes to #62 and #127

@amartya4256 amartya4256 marked this pull request as ready for review January 9, 2025 13:33
Copy link
Contributor

github-actions bot commented Jan 9, 2025

Test Results

   486 files  ±0     486 suites  ±0   10m 12s ⏱️ + 1m 35s
 4 309 tests +1   4 296 ✅ +1   13 💤 ±0  0 ❌ ±0 
16 414 runs  +1  16 306 ✅ +1  108 💤 ±0  0 ❌ ±0 

Results for commit 6f047a5. ± Comparison against base commit cec96e9.

♻️ This comment has been updated with latest results.

@amartya4256 amartya4256 force-pushed the get_imageData_for_any_zoom branch 2 times, most recently from a2785fe to 9a9747e Compare January 9, 2025 15:13
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.

It seems to me as if I had misunderstood the original intent of this change. It does not seem to be an actual bugfix but rather an improvement regarding which image data (more particularly, the image data at which scaling) to use for generating the one at the requested zoom level of. Is that correct?

Then of course the kind of test I proposed and is provided in this PR is not a regression test for the change. And probably it's something that we do not even need to test in that way at all.

@amartya4256 amartya4256 force-pushed the get_imageData_for_any_zoom branch from 9a9747e to 37514eb Compare January 13, 2025 13:34
@amartya4256
Copy link
Contributor Author

It seems to me as if I had misunderstood the original intent of this change. It does not seem to be an actual bugfix but rather an improvement regarding which image data (more particularly, the image data at which scaling) to use for generating the one at the requested zoom level of. Is that correct?

Then of course the kind of test I proposed and is provided in this PR is not a regression test for the change. And probably it's something that we do not even need to test in that way at all.

In the previous implementation in case of integer 200 scaling, we only allowed the clients to retrieve the imageData of corrected zoom since our initial expectations were that we don't allow images to be created at, for example, the quarter zoom if the quarter scaling is disabled. But since we know that in some cases like Program icons we can get handles of image at these zoom levels, it doesnt make sense to restrict clients from requesting imagedata at certain zoom levels. In other words, this is indeed a feature enhancement as we are now allowing to store any zoom level in the handles map and retrieve them on demand.

Hence, I would not consider this change as a bugfix but rather an improvement. But the tests do make sense to me since it validates the behaviour of getImageData at random zoom levels. It would not be considered a regression test but a feature test.

This contribution helps retrieve the imageData for the
requested zoom by searching for the zoom in the zoomToHandleMap
followed by (if not available) scaling the handle at the zoom nearest to
it while getting rid of only searching among the autoscaled zoom values.

Contributes to eclipse-platform#62 and eclipse-platform#127
@HeikoKlare HeikoKlare force-pushed the get_imageData_for_any_zoom branch from 37514eb to 6f047a5 Compare January 13, 2025 19:32
@HeikoKlare HeikoKlare merged commit c49cee7 into eclipse-platform:master Jan 13, 2025
14 checks passed
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.

2 participants