Skip to content

Replacing Image(Device, ImageData) constructor in DefaultRangeIndicator with Image(Device, ImageDataProvider) #3004

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

Merged

Conversation

arunjose696
Copy link

DefaultRangeIndicator previously used Image(Device, ImageData) constructor to draw a grid to display the selected range.

Previously, the grid was rendered using a 1×1 pixel checkerboard pattern (blue and white), which was mainly inteded to work at 100% zoom. However, at higher zoom levels (e.g., 150%, 200%), the visual intensity of the pattern becomes different, causing the grid to appear visually inconsistent across zooms.

Figures below uses Image(Device, ImageData)


200%

100%

The new implementation uses Image(Device, ImageDataProvider) adapts the grid's block size based on the zoom scale. Instead of always using 1×1 blocks, it calculates the block size by rounding the zoom factor:

For zoom levels like 100%, 125%, etc., block size = 1 (no change).

For higher zoom levels like 150%, 200%, etc., block size = 2.

This means the grid will now use 2×2 rectangles for higher zooms, resulting in a visual appearance that better matches how the original 1×1 pattern looks on standard (100%) displays. This preserves visual consistency across different zoom levels and monitor scaling settings.

Figures below Uses Image(Device, ImageDataProvider)


100%

125%

150%

175%

200%

Contributes to vi-eclipse/Eclipse-Platform#199

@arunjose696 arunjose696 force-pushed the bug/arun/199/rangeIndicator branch 2 times, most recently from 047fde5 to 8f34f2e Compare May 22, 2025 14:03
Copy link
Contributor

github-actions bot commented May 22, 2025

Test Results

 2 778 files  ±0   2 778 suites  ±0   1h 35m 3s ⏱️ - 2m 37s
 7 928 tests +1   7 699 ✅ +1  228 💤 ±0  1 ❌ ±0 
23 337 runs  +3  22 590 ✅ +3  746 💤 ±0  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit 4e03f11. ± Comparison against base commit a9213eb.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

The code works, just some minor comments regarding coding practices.

@arunjose696 arunjose696 force-pushed the bug/arun/199/rangeIndicator branch 4 times, most recently from 90e117f to 52891ee Compare June 4, 2025 09:46
@arunjose696 arunjose696 force-pushed the bug/arun/199/rangeIndicator branch from 52891ee to 56e62ca Compare June 5, 2025 13:57
Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

Last change: please adapt the commit message so that the 1st line has <=80 characters.

Something like:

Use constructor Image(Device, ImageDataProvider) in DefaultRangeIndicator

The rest of the commit message is fine, just the 1st line is too long.

…ator

DefaultRangeIndicator previously used  Image(Device, ImageData) constructor to draw a grid to display the selected range.
Previously, the grid was rendered using a 1×1 pixel checkerboard pattern
(blue and white), which was mainly inteded to work at 100% zoom.
However, at higher zoom levels (e.g., 150%, 200%), the visual intensity
of the pattern becomes different, causing the grid to appear visually
inconsistent across zooms.
@arunjose696 arunjose696 force-pushed the bug/arun/199/rangeIndicator branch from 56e62ca to 8359bf5 Compare June 12, 2025 09:08
@arunjose696
Copy link
Author

Use constructor Image(Device, ImageDataProvider) in DefaultRangeIndicator

Done

@eclipse-platform-bot
Copy link
Contributor

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

bundles/org.eclipse.ui.workbench.texteditor/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch
From f25918e0bc6fa2efdbf56f3a9c72ac1a06669541 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Thu, 12 Jun 2025 09:43:17 +0000
Subject: [PATCH] Version bump(s) for 4.37 stream


diff --git a/bundles/org.eclipse.ui.workbench.texteditor/META-INF/MANIFEST.MF b/bundles/org.eclipse.ui.workbench.texteditor/META-INF/MANIFEST.MF
index 6b6a3331b6..9d8d7de9aa 100644
--- a/bundles/org.eclipse.ui.workbench.texteditor/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.ui.workbench.texteditor/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.ui.workbench.texteditor; singleton:=true
-Bundle-Version: 3.19.200.qualifier
+Bundle-Version: 3.19.300.qualifier
 Bundle-Activator: org.eclipse.ui.internal.texteditor.TextEditorPlugin
 Bundle-ActivationPolicy: lazy
 Bundle-Vendor: %providerName
-- 
2.49.0

Further information are available in Common Build Issues - Missing version increments.

@fedejeanne
Copy link
Contributor

I just manually bumped the version, like the bot was supposed to do on its own (see #3004 (comment)).

@HannesWell just out of curiosity, do you have any idea what could have gone wrong? Is it maybe because Arun is not a contributor yet and I need to approve the workflow runs?

@HeikoKlare
Copy link
Contributor

I just manually bumped the version, like the bot was supposed to do on its own (see #3004 (comment)).

HannesWell just out of curiosity, do you have any idea what could have gone wrong?

That's a known limitation: the bot cannot push to forks of organizations. I think it's not even related to the bot but it's the same for any other maintainer.

@fedejeanne
Copy link
Contributor

The check failures are unrelated and the flaky test has been documented in #3042

@fedejeanne fedejeanne merged commit 033b4c2 into eclipse-platform:master Jun 13, 2025
17 of 22 checks passed
@fedejeanne fedejeanne deleted the bug/arun/199/rangeIndicator branch June 13, 2025 09:38
@HannesWell
Copy link
Member

I just manually bumped the version, like the bot was supposed to do on its own (see #3004 (comment)).
HannesWell just out of curiosity, do you have any idea what could have gone wrong?

That's a known limitation: the bot cannot push to forks of organizations. I think it's not even related to the bot but it's the same for any other maintainer.

Exactly. I just created eclipse-platform/eclipse.platform.releng.aggregator#3136 to warn in case as it's a reoccurring issue.

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.

Evaluate calling places of Image constructors with ImageData
5 participants