Skip to content

Conversation

@HeikoKlare
Copy link
Contributor

This increase precision of size computation by reducing the number of pixel/point conversions. In particular, the minimumSize() method unnecessarily calculated in pixels even though all relevant values are in points. Inside the computeSizeInPixels() methods, several conversions were done as most other called methods operate on point values.

I was not able to demonstrate benefits of the higher precision yet, but it anyway cleans up the code by removing unnecessary conversions.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 30, 2025

Test Results

  118 files  ±0    118 suites  ±0   16m 21s ⏱️ +2s
4 650 tests ±0  4 633 ✅ ±0  17 💤 ±0  0 ❌ ±0 
  330 runs  ±0    326 ✅ ±0   4 💤 ±0  0 ❌ ±0 

Results for commit 795e239. ± Comparison against base commit c3318b8.

♻️ This comment has been updated with latest results.

@HeikoKlare HeikoKlare marked this pull request as ready for review October 30, 2025 18:45
@HeikoKlare HeikoKlare force-pushed the computesize-point-values branch from 54b1e7f to 918b82c Compare October 31, 2025 08:50
Copy link
Contributor

@amartya4256 amartya4256 left a comment

Choose a reason for hiding this comment

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

The changes look reasonable. Passing the point value to computeSizeInPixels significantly reduces the amount of DPIUtil.pixelToPoint calls inside the method. The code looks sound and doesn't break anything upon running. I have tested it on zoom level 250%, 150% and 175%. Is it possible to add a unit test to test the precision? I'm approving it already.

@HeikoKlare
Copy link
Contributor Author

Is it possible to add a unit test to test the precision?

I did not find a way to demonstrate the higher precision yet. Probably you can produce some scenario with specific zoom and size values and an according layout involved, but even if you found such a scenario, the test for it would be very specific. I would rather flag this as a simplification with the ability to improve precision without actually claiming the latter.

@amartya4256
Copy link
Contributor

Is it possible to add a unit test to test the precision?

I did not find a way to demonstrate the higher precision yet. Probably you can produce some scenario with specific zoom and size values and an according layout involved, but even if you found such a scenario, the test for it would be very specific. I would rather flag this as a simplification with the ability to improve precision without actually claiming the latter.

I was just wondering if there's a direct way to test that. But its fine without.

@akoch-yatta akoch-yatta force-pushed the computesize-point-values branch from 918b82c to 9171d8c Compare October 31, 2025 14:34
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.

Good looks good. Only have nitpicky comments

This increase precision of size computation by reducing the number of
pixel/point conversions. In particular, the minimumSize() method
unnecessarily calculated in pixels even though all relevant values are
in points. Inside the computeSizeInPixels() methods, several conversions
were done as most other called methods operate on point values.
@HeikoKlare HeikoKlare force-pushed the computesize-point-values branch from 9171d8c to 795e239 Compare October 31, 2025 15:12
@akoch-yatta akoch-yatta merged commit 704dc92 into eclipse-platform:master Oct 31, 2025
17 checks passed
@akoch-yatta akoch-yatta deleted the computesize-point-values branch October 31, 2025 15:37
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.

Unnecessary pixel/point conversions in size computations

3 participants