-
Notifications
You must be signed in to change notification settings - Fork 190
Sharp rendering of SVGs passed as InputStream #2935
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?
Conversation
|
@ShahzaibIbrahim please provide a consistent PR description and at best show before/after screenshot. |
Hi @laeubi, This PR is not ready and hence it's currently drafted. |
I appreciate that it's marked as a draft, but even draft PRs should contain a clear title and meaningful description explaining what you're working on and what the expected outcome is. This helps maintainers understand the context and provide better guidance early in the process. The SWT repository should have draft PRs that clearly communicate:
If you need to run CI checks while developing, you can do that on your fork as well. Please update the PR description before continuing, so we can provide better feedback. Thanks! |
9d99069 to
3f0b54c
Compare
I have updated the PR description with all the sufficient information I currently have. |
| /** | ||
| * Byte array to store input stream data to draw on various Zoom levels | ||
| */ | ||
| private byte[] inputStreamData; |
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.
Would this not better be part of a supplier instead of part of the image?
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 need o make it part of Image class because I have to use it in private class CachedImageAtSize#loadImageDataAtExactSize not just in initWithSupplier. Or do you mean something else?
|
Some early feedback on Mac for this one... Here's a screenshot running this Snippet of the changes in this PR and #2924
@ShahzaibIbrahim Thank-you for your work on this and #2924 |
25abe2b to
e4cd2a2
Compare
|
@HeikoKlare I think tests are fixed. Can you please test my snippet on multiple zoom level on mac to mark this PR ready for review? |
Storing input stream content to be later used for executeOnImageHandleAtBestFittingSize() to sharply render the svg images when created using Image(Display, InputStream)
e4cd2a2 to
4cdd05f
Compare
HeikoKlare
left a comment
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.
Following up on the comment of @laeubi, wouldn't it be cleaner to wrap the input stream handling in an ImageDataProvider to not further mess up the Image implementation with mutual exclusive providers?
It might work as follows (starting from the InputStream constructor):
- Read stream data from stream
- Check if stream data belong to a dynamically sizable image, in that case create an
ImageDataAtSizeProviderotherwise create anImageDataProvider.- The
ImageDataAtSizeProviderwill store the stream data and return accordingly loaded image data in itsgetImageData(width, height)andgetImageData(zoom)methods - The
ImageDataProviderwill only return according image data at 100% and none for the other zoom, so it does not need to store the stream data
- The
Nothing needs to be changed for the loadImageDataAtExactSize then, as it reuses the ImageDataAtSizeProvider functionality.
| initDataFromInputStream(currentDeviceZoom); | ||
| init(); | ||
| } catch (IOException e) { | ||
| SWT.error(SWT.ERROR_INVALID_ARGUMENT, e); |
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.
This is not the correct error w.r.t. the contract. It should be SWT.ERROR_IO.
| } | ||
| } else if (inputStreamData != null) { | ||
| int deviceZoomLevel = deviceZoom; | ||
| if (deviceZoomLevel != currentDeviceZoom) { |
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.
One could think of checking for isDynamicallySizable here, as otherwise you will unnecessary destroy and recreate the exact same handle. However, it's not done for the other providers as well and with my general remark, this will not be easily possible either.
| import org.eclipse.swt.layout.*; | ||
| import org.eclipse.swt.widgets.*; | ||
|
|
||
| public class SVGImageTests { |
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 is already Snippet390, so this one is obsolete.
| private ImageGcDrawer imageGcDrawer; | ||
|
|
||
| /** | ||
| * Byte array to store input stream data to draw on various Zoom levels |
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 is no drawing happening with input streams (same for the Cocoa implementation).
| * Byte array to store input stream data to draw on various Zoom levels | |
| * Byte array to store input stream data to convert into image data at various Zoom levels |


Storing input stream content to be later used for executeOnImageHandleAtBestFittingSize() to sharply render the svg images when created using Image(Display, InputStream)
In addition, on Linux an image can be refreshed for a new zoom (Image#refreshImageForZoom()), which was also not supported for the case that the image is based on an InputStream. Since we are storing the input stream content anyway, this case is should be covered here too.
Contribute to: #2917
How to Test
Use this SVG image for the Snippet:
Results
Without this PR:

After Changes:
GTK:
Cocoa:
To be Tested