Skip to content

Conversation

@HeikoKlare
Copy link
Contributor

The GTK implementation of the Image class currently contains the basically same piece of code for initializing and image based on an ImageFileNameProvider and ImageDataProvider twice, once in the Image constructors and once in the refresh logic for changing the zoom.

This change factors out the common functionality in reused methods and streamlines the implementation for the ImageFileNameProvider.

This serves as a preparation for:

Unifies the GTK with the Windows implementation as adapted in:

@HeikoKlare HeikoKlare force-pushed the gtk-image-initializations branch from 2dcd231 to a5dbf87 Compare March 10, 2025 19:57
@github-actions
Copy link
Contributor

github-actions bot commented Mar 10, 2025

Test Results

   506 files  ±0     506 suites  ±0   8m 51s ⏱️ +33s
 4 341 tests ±0   4 327 ✅ ±0   14 💤 ±0  0 ❌ ±0 
16 597 runs  ±0  16 488 ✅ ±0  109 💤 ±0  0 ❌ ±0 

Results for commit dc5cebc. ± Comparison against base commit 632681f.

♻️ This comment has been updated with latest results.

@HeikoKlare HeikoKlare marked this pull request as ready for review March 10, 2025 20:35
@HeikoKlare
Copy link
Contributor Author

@akoch-yatta May I ask you to also have a look at this change? It is quite similar to the Windows one from last week, except for the GTK implementation still being tied to a single handle (thus destroying handles upon zoom changes).

I have already tested the change on a Linux system with different and changing monitor zoom values, so it's primarily about checking soundness and correctness for the code.

Copy link
Contributor

@akoch-yatta akoch-yatta left a comment

Choose a reason for hiding this comment

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

Apart from the one remark, this change looks good to me

}
if (this.surface == 0) {
ImageData imageData = new ImageData(fileForZoom.element());
if (fileForZoom.zoom() == zoom) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That must be an !=

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thank you! That was part of a final small refactoring after the lastest test. Always good to check again afterwards...

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that situation 😄 PR looks good to me now

@HeikoKlare HeikoKlare force-pushed the gtk-image-initializations branch 2 times, most recently from 01ae47e to 0a1f0c4 Compare March 11, 2025 09:27
@HeikoKlare HeikoKlare requested a review from akoch-yatta March 11, 2025 09:29
The GTK implementation of the Image class currently contains the
basically same piece of code for initializing and image based on an
ImageFileNameProvider and ImageDataProvider twice, once in the Image
constructors and once in the refresh logic for changing the zoom.

This change factors out the common functionality in reused methods and
streamlines the implementation for the ImageFileNameProvider.
@HeikoKlare HeikoKlare force-pushed the gtk-image-initializations branch from 0a1f0c4 to dc5cebc Compare March 11, 2025 10:08
@HeikoKlare HeikoKlare merged commit 326658a into eclipse-platform:master Mar 11, 2025
14 checks passed
@HeikoKlare HeikoKlare deleted the gtk-image-initializations branch March 11, 2025 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants