Use the existing image to initialize the GC in ImageGcDrawerWrapper #2238
Conversation
ShahzaibIbrahim
force-pushed
the
master-328
branch
from
20cf50a to
a8d8a82
Compare
Test Results 536 files - 9 536 suites - 9 30m 39s ⏱️ - 1m 29s For more details on these failures and errors, see this check. Results for commit 24fbd72. ± Comparison against base commit cad131b. This pull request removes 74 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
I've tested the behavior with the LineNumberRuler and that seems to be work fine with different zooms. But with this change we will see the following warnings when strict checks are enabled, which need to be resolved:
***WARNING: Images with handles created for multiple zooms should not be modified. It should be created with an ImageGcDrawer (see SWT Snippet 384).
| GC gc = new GC(new DrawableWrapper(image, zoom), gcStyle); | ||
| createBaseHandle(zoom, width, height); | ||
| }; | ||
| GC gc = new GC(new DrawableWrapper(Image.this, zoom), gcStyle); |
There was a problem hiding this comment.
Can't we then remove the DrawableWrapper and just call?
| GC gc = new GC(new DrawableWrapper(Image.this, zoom), gcStyle); | |
| GC gc = new GC(Image.this, gcStyle); |
There was a problem hiding this comment.
There's a scenario where it's needed when another GC can be called over this image from the outside. I am not 100% of the scenario but maybe @akoch-yatta can explain it bit better on why it was used in the first place.
|
Is this something we can only do for the Windows implementation or may we adapt all the MacOS and Linux implementations of |
This error should go away once this PR is merged. |
Yes, it make sense to adapt the MacOS and Linux implementation of Image. But since I can't test the changes, I will wait until @fedejeanne is back and can test those changes with him. |
In ImageGCDrawerWrapper#newImageHandle, we create a new image with existing data to initialize the GC, instead we could use the existing image object and the base handle should be created as it is done in PlainImageDataProvider.
ShahzaibIbrahim
force-pushed
the
master-328
branch
from
d8bdef8 to
24fbd72
Compare
| ImageData imageData = Image.this.getImageMetadata(zoom).getImageData(); | ||
| drawer.postProcess(imageData); | ||
| ImageData newData = adaptImageDataIfDisabledOrGray(imageData); | ||
| return init(newData, zoom); | ||
| if(imageData.data != null) { | ||
| zoomLevelToImageHandle.get(zoom).destroy(); | ||
| init(imageData, zoom); | ||
| } |
There was a problem hiding this comment.
Here I want initialize the imageData only if postProcess is called. That's why I have a default implementation that nullify the imageData in order to know here if it was not implemented. I know it's not a clean solution and I am open to hear suggestion.
My suggestions:
- Have some field to track if postProcess was called.
- Always call init regardless of postProcess calling.
In ImageGCDrawerWrapper#newImageHandle, we create a new image with existing data to initialize the GC, instead we could use the existing image object and the base handle should be created as it is done in PlainImageDataProvider.