Skip to content
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

Implementation of Text Layout to enable multi zoom level support for win32 #1244

Merged

Conversation

amartya4256
Copy link
Contributor

@amartya4256 amartya4256 commented May 21, 2024

Addressed issues

Requires

Note: Only the last commit in this PR is to be reviewed. Previous commit(s) belong to the prerequisite PR(s)

Description

This pull request is based on the implementations of PR #1243. This commit adds the support of scaling of text layout based on the zoom level of the monitor it is drawn on. It uses the native zoom which is provided by the GCData if available otherwise the zoom level set in the font (since the right font for a zoom level is computed already at the control level, so we always have the consistent zoom information).

@amartya4256 amartya4256 requested a review from niraj-modi as a code owner May 21, 2024 12:43
@amartya4256
Copy link
Contributor Author

@HeikoKlare @fedejeanne @akoch-yatta Please have a look.

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.

These changes are quite hard to review, so I am wondering whether we can split them up to reduce the (accidental) complexity. Would it be possible to split this up into two subsequent commits/PRs like:

  1. Replace the handling of InPixel values but still using DPIUtil's autoscale method
  2. Replace the autoScale calls by scale calls passing the actual zoom

@amartya4256 amartya4256 force-pushed the multi_zoom_level_text_layout branch 5 times, most recently from b64709e to fc50383 Compare May 29, 2024 14:49
@amartya4256 amartya4256 force-pushed the multi_zoom_level_text_layout branch 3 times, most recently from d8ae71c to 8de0a67 Compare June 7, 2024 12:17
Copy link
Contributor

github-actions bot commented Jun 7, 2024

Test Results

   450 files  +8     450 suites  +8   7m 57s ⏱️ -5s
 4 127 tests +1   4 119 ✅ +1   8 💤 ±0  0 ❌ ±0 
16 319 runs  +1  16 227 ✅ +1  92 💤 ±0  0 ❌ ±0 

Results for commit 8c52ca4. ± Comparison against base commit e0356e9.

♻️ This comment has been updated with latest results.

@amartya4256 amartya4256 force-pushed the multi_zoom_level_text_layout branch 3 times, most recently from 54d9dd8 to aadd1d9 Compare June 11, 2024 14:27
@fedejeanne
Copy link
Contributor

Fixed the issue with the icon of the shell. But I have also created a follow up issue which addresses the problem in the depth but it doesn't have to be the part of this PR because it addresses the usability of a method outside its scope. https://github.com/orgs/vi-eclipse/projects/1/views/1?filterQuery=assignee%3Aamartya4256+47&pane=issue&itemId=67443386 FYI @akoch-yatta @fedejeanne @HeikoKlare

Do you mean this one? #1236 (review)

@amartya4256 amartya4256 force-pushed the multi_zoom_level_text_layout branch from aadd1d9 to 586d805 Compare June 14, 2024 13:12
@amartya4256
Copy link
Contributor Author

Fixed the issue with the icon of the shell. But I have also created a follow up issue which addresses the problem in the depth but it doesn't have to be the part of this PR because it addresses the usability of a method outside its scope. https://github.com/orgs/vi-eclipse/projects/1/views/1?filterQuery=assignee%3Aamartya4256+47&pane=issue&itemId=67443386 FYI @akoch-yatta @fedejeanne @HeikoKlare

Do you mean this one? #1236 (review)

Yeah sorry I deleted my comment and moved it to the image PR

@amartya4256 amartya4256 force-pushed the multi_zoom_level_text_layout branch from 586d805 to 5ec74cb Compare June 14, 2024 13:20
@fedejeanne fedejeanne dismissed their stale review June 17, 2024 08:42

All my comments have been addressed. This PR is ready to be tested, I'll do it in the afternoon.

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.

Tests are not running.

@fedejeanne
Copy link
Contributor

I tested this and I didn't find any regressions. As soon as the tests run in the suite (see #1244 (comment)) and all open comments from Heiko and Andreas have been marked as resolved, I will merge this PR.

@amartya4256 amartya4256 force-pushed the multi_zoom_level_text_layout branch from 5ec74cb to 3839910 Compare June 17, 2024 09:53
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.

Copyright header is missing.

I also restarted the build to see if the failed tests are fixed (see my response to #1244 (comment))

@amartya4256 amartya4256 force-pushed the multi_zoom_level_text_layout branch from 3839910 to 90e8456 Compare June 17, 2024 10:13
This commit adds the support of scaling of text layout based on the zoom
level of the monitor it is drawn on. It uses the native zoom which is
provided by the GCData if available otherwise the zoom level set in the
font (since the right font for a zoom level is computed already at the
control level, so we always have the consistent zoom information).

contributes to eclipse-platform#62 and eclipse-platform#127
@amartya4256 amartya4256 force-pushed the multi_zoom_level_text_layout branch from 90e8456 to 8c52ca4 Compare June 17, 2024 10:14
@fedejeanne fedejeanne merged commit 7ac43c0 into eclipse-platform:master Jun 17, 2024
14 checks passed
amartya4256 added a commit to amartya4256/eclipse.platform.swt that referenced this pull request Jun 17, 2024
This commit is a follow up to the PR eclipse-platform#1244 which improves the
readability of the class TextLayout for win32. The field lineWidth is
now renamed to lineWidthInPixels since the field represented the
information in pixels.

contributes to eclipse-platform#62 and eclipse-platform#127
amartya4256 added a commit to amartya4256/eclipse.platform.swt that referenced this pull request Jun 17, 2024
This commit is a follow up to the PR eclipse-platform#1244 which improves the
readability of the class TextLayout for win32. The field lineWidth is
now renamed to lineWidthInPixels since the field represented the
information in pixels.

contributes to eclipse-platform#62 and eclipse-platform#127
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.

4 participants