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

Refactor Image for storing ImageMetadata #1496

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Refactor Image for storing ImageMetadata #1496

wants to merge 2 commits into from
+521 −481

Conversation

amartya4256
Copy link
Contributor

@amartya4256 amartya4256 commented Sep 26, 2024 •
edited by fedejeanne
Loading

This PR contributes to the refactoring of Image class for win32.

The goal is to store more than just the handle of the image for each zoom level. To do that, a new private class is created (it's called ImageHandle) and an image contains one instance of this new class for each zoom level. The class contains the following fields:

  • handle
  • height
  • width

contributes to #62 and #127

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 26, 2024 •
edited
Loading

Test Results

   482 files   -  1     482 suites   - 1   8m 43s ⏱️ - 3m 51s
 4 060 tests  - 35   4 052 ✅  - 33   4 💤  - 3  3 ❌ ±0  1 🔥 +1 
16 138 runs   - 35  16 047 ✅  - 33  87 💤  - 3  3 ❌ ±0  1 🔥 +1 

For more details on these failures and errors, see this check.

Results for commit 5d4076a. ± Comparison against base commit e6588c2.

This pull request removes 35 tests. 
org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageShouldHaveDimesionAsPerZoomLevel
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testByteArrayTransfer
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testFileTransfer
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testHtmlTransfer
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromCopiedImage
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImage
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageData
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageDataFromImage
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testRtfTransfer
org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testTextTransfer
…

♻️ This comment has been updated with latest results.

@amartya4256 amartya4256 marked this pull request as ready for review September 30, 2024 11:39
@amartya4256 amartya4256 force-pushed the imageMetadata branch 2 times, most recently from 3832a24 to e353b1a Compare September 30, 2024 13:36
@amartya4256 amartya4256 force-pushed the imageMetadata branch 3 times, most recently from 8067b98 to 6225eae Compare October 10, 2024 14:29
@akoch-yatta akoch-yatta linked an issue Oct 30, 2024 that may be closed by this pull request
Refactor Image for Multi-Zoom Support vi-eclipse/Eclipse-Platform#122
Open
@fedejeanne fedejeanne mentioned this pull request Oct 30, 2024
Open
@fedejeanne fedejeanne marked this pull request as draft October 31, 2024 09:59
@fedejeanne
Copy link
Contributor

Converted it back to Draft, this one should be merged in 4.35 M1

@amartya4256 amartya4256 force-pushed the imageMetadata branch 2 times, most recently from 689b931 to 42db14c Compare November 12, 2024 13:53
fedejeanne
fedejeanne reviewed Nov 12, 2024
View reviewed changes
Copy link
Contributor

@fedejeanne fedejeanne left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have some comments regarding coding style and one missing return

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
if (zoom != null && !zoomLevelToHandle.containsKey(zoom)) {
zoomLevelToHandle.put(zoom, handle);
private void setImageMetadataForHandle(ImageHandle imageMetadata, Integer zoom) {
if (zoom != null && !zoomLevelToImageHandle.containsKey(zoom)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be a proper response to throw an exception if someone tries to override the existing metadata for a given zoom ? This way you are silently ignoring the case, which may have unintended consequences in the future.

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Outdated Show resolved Hide resolved
@amartya4256 amartya4256 force-pushed the imageMetadata branch 3 times, most recently from 1edd016 to 703bf95 Compare November 14, 2024 11:08
@amartya4256
Image handle refactoring for win32
83bd8d7 
This contribution encapsulates the metadata of the image in an
innerclass ImageHandle which is used to create a hashmap of zoom level
to imageHandle object inside an image object, making it straight
forward to obtain any metadata information from an image for a zoom
level.

contributes to eclipse-platform#62 and eclipse-platform#127
@amartya4256 amartya4256 force-pushed the imageMetadata branch 2 times, most recently from a575ad0 to ea52af4 Compare November 14, 2024 14:25
@amartya4256
Refactoring ImageProviders
5d4076a 
This commit refactors usage of ImageDataProvider and
ImageFilenameProvider to make the code more readable.

contributes to eclipse-platform#62 and eclipse-platform#127
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akoch-yatta akoch-yatta akoch-yatta left review comments

@fedejeanne fedejeanne fedejeanne left review comments

@niraj-modi niraj-modi Awaiting requested review from niraj-modi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor Image for Multi-Zoom Support
3 participants
@amartya4256 @fedejeanne @akoch-yatta