-
Notifications
You must be signed in to change notification settings - Fork 176
A strict check 200% image data is double the size of 100% image data #2249
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
base: master
Are you sure you want to change the base?
A strict check 200% image data is double the size of 100% image data #2249
Conversation
Test Results 539 files - 6 539 suites - 6 27m 41s ⏱️ - 1m 13s For more details on these errors, see this check. Results for commit c40fee2. ± Comparison against base commit 6842283. This pull request removes 37 tests.
♻️ This comment has been updated with latest results. |
a31c3d0
to
9a302a0
Compare
560fc50
to
085c1c6
Compare
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/DPIUtil.java
Outdated
Show resolved
Hide resolved
085c1c6
to
d78d6ae
Compare
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
Outdated
Show resolved
Hide resolved
db68f87
to
1403084
Compare
In CI failing checks I have the below error, Dont think this has anything to do with my changes.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments before I move to the testing phase
...e.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java
Outdated
Show resolved
Hide resolved
...t.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_ImageData.java
Outdated
Show resolved
Hide resolved
...sts/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_internal_SVGRasterizer.java
Outdated
Show resolved
Hide resolved
...sts/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_internal_SVGRasterizer.java
Outdated
Show resolved
Hide resolved
...t.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_ImageData.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/common/org/eclipse/swt/internal/DPIUtil.java
Outdated
Show resolved
Hide resolved
8b2a4c3
to
73e1a6c
Compare
982ab6d
to
a6d4f74
Compare
} | ||
|
||
if (scaledImageData.width != scaleFactor * baseImageData.width || scaledImageData.height != scaleFactor * baseImageData.height) { | ||
SWT.error(SWT.ERROR_INVALID_ARGUMENT, null, "ImageData should be linearly scaled across zooms."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced exception with a System.err.println()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case you'll have to remove the 2 new tests since there is no way to test that something was written to the console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And you should adapt the text of this PR since nothing fails, one simply sees some output in the console. Please also add a snippet to test by providing 2 image files, one of them (the _@2x
) having the wrong size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve updated the PR text . I also added the two snippets one with an ImageDataProvider that scales the image data correctly and one that doesn’t.
To keep the setup simple, I didn’t use actual image files (e.g. SVGs), since that would require uploading multiple files and slightly complicate the example. Let me know if including real images is still necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is necessary, that is a valid use case and it should be tested too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In PR text I’ve now added tests using actual .png image files in addition to the earlier code snippets.
I’m still a bit unclear on why this was necessary, as I thought the original snippets already demonstrated the intended behavior with the case where ImageData is unscaled. That said, I’ve still added a snippet with real images as requested. Please let me know if this is what you had in mind, or if there’s another use case I should be covering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I had the wrong expectation about this PR.
FTR what I wanted to test is in JFace and you could see it for example by adding a modified version of validateLinearScaling
that takes ImageDescriptor
as a parameter and calling it at the end of org.eclipse.jface.tests.images.FileImageDescriptorTest.testAdaptToURL()
. If you do that and provide a second file that adds @2x
to the end of the file but it does not have double the size then the check would fail.
Since I'm not on Windows at the moment, I can't test this PR- I'll get back to it later this week.
7762a2a
to
bc5eaca
Compare
A strict check to be used only for debugging means, this ensures the imageData is scaled linearly for 100 and 200 zooms.
bc5eaca
to
c40fee2
Compare
A strict check to be used only for debugging means, this ensures the imageData is scaled linearly for 100 and 200 zooms.
Steps to reproduce
Out of the blow snippets. Snippet1 which uses an image data provider that always returns the same image, when executed with the VM argument
-Dorg.eclipse.swt.internal.enableStrictChecks=True
, logs the error` "ImageData should be linearly scaled across zooms." to stdout. Whereas snippet 2 which scales ImageData linearly runs without this error log.snippet 1
snippet 2
The below examples demonstrate testing this again with PNG images. In the first snippet, the ImageDataProvider returns the image data from the same file for both 100% and 200% zoom levels without scaling, which causes the error message "ImageData should be linearly scaled across zooms." to be printed to stdout. In the second snippet it provides properly scaled image data at 200% zoom (using a scaled image), so it executes without logging this error.
To run these snippets, download the following PNG files and place them in your working directory:
pause.png
[email protected]
snippet 1
snippet 2