Skip to content

Use the existing image to initialize the GC in ImageGcDrawerWrapper #2238

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use the existing image to initialize the GC in ImageGcDrawerWrapper #2238

wants to merge 1 commit into from

Conversation

ShahzaibIbrahim
Copy link
Contributor

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 ShahzaibIbrahim linked an issue Jun 16, 2025 that may be closed by this pull request
Use the existing image to initialize the GC in ImageGcDrawerWrapper vi-eclipse/Eclipse-Platform#328
Open
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 16, 2025 •
edited
Loading

Test Results

   546 files  ±0     546 suites  ±0   29m 58s ⏱️ -18s
 4 426 tests ±0   4 409 ✅ ±0   17 💤 ±0  0 ❌ ±0 
16 750 runs  ±0  16 623 ✅ ±0  127 💤 ±0  0 ❌ ±0 

Results for commit 36a5fce. ± Comparison against base commit 6b5b54a.

♻️ This comment has been updated with latest results.

HeikoKlare
HeikoKlare reviewed Jun 16, 2025
View reviewed changes
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.

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).

@HeikoKlare
Copy link
Contributor

Is this something we can only do for the Windows implementation or may we adapt all the MacOS and Linux implementations of Image as well?

@ShahzaibIbrahim
Copy link
Contributor Author

***WARNING: Images with handles created for multiple zooms should not be modified. It should be created with an ImageGcDrawer (see SWT Snippet 384).

This error should go away once this PR is merged.

@ShahzaibIbrahim
Copy link
Contributor Author

Is this something we can only do for the Windows implementation or may we adapt all the MacOS and Linux implementations of Image as well?

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.

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-328 branch 7 times, most recently from 099e31c to d96e9ef Compare August 13, 2025 07:52
@ShahzaibIbrahim
Use the existing image to initialize the GC in ImageGcDrawerWrapper
36a5fce 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HeikoKlare HeikoKlare HeikoKlare left review comments

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

Successfully merging this pull request may close these issues.

Use the existing image to initialize the GC in ImageGcDrawerWrapper
2 participants
@ShahzaibIbrahim @HeikoKlare