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

Allowing to define variable line heights #233110

Open
wants to merge 68 commits into
base: main
Choose a base branch
from
Open

Allowing to define variable line heights #233110

wants to merge 68 commits into from

Conversation

aiday-mar
Copy link
Contributor

@aiday-mar aiday-mar commented Nov 5, 2024

fixes #147067

Test pass 1:

Verify the following cases:

  • When several different line heights are defined on the same line, the maximum line height is chosen
  • When testing on checker.ts that has 100k lines and has many special line heights (the class definitions are rendered with 100px height) the lines are rendered quickly
  • Verify the last line height is rendered correctly if set
  • Check that the ghost text is positioned correctly in the center of the line
  • Check that git blame is positioned correctly in the center of the line
  • Check that the git lens decorations are positioned correctly above the line
  • Verify that completions box is positioned correctly
  • Verify that gutter lines are correctly extended across the whole line height
  • Whitespace markers are correctly positioned
  • We are able to select words as before
  • Other occurences highlighted background is correctly rendered
  • Verify the light bulb is correctly positioned.
    • Light bulb renders on the same line at the top of the line, so not aligned with the text, but it looks okay. The light bulb does not need to be aligned, because the font size can also be variable (future work).
  • Verify that cmd+hover makes correct squiggle appear
    • It appears below the text
  • Verify context menu is positioned correctly
    • Context menu always appears at the cursor position
  • Verify that parameter hints are displayed correctly on a line with different line height
    • Parameter hints are positioned in the center vertically like the text
  • Verify that changing the theme does not pose any difference
    • Theme does not change anything
  • Verify indentation guides are correctly positioned
    • Indentation guides are correctly positioned.
  • Check that command Show Editor Context Menu makes context menu appear at the correct location
  • Verify that breakpoints are positioned in the centre of the gutter as expected
  • Verify that IME input also positioned correctly
    • When accessibility is turned on
    • When accessibility is turned off
  • Verify that sticky scroll rendering is correct.
    • folding icon is correctly positioned
    • line height corresponds to the editor line height
  • Verify that when changing the cursor style, the cursor rendering is correct
    • line
    • block
    • underline
    • line-thin
    • block-outline
    • underline-thin
  • Verify that the editor decorations are correctly synchronized with the line heights map in the linesLayout file
  • Verify that when you remove all the text on that line, line height changes correctly
  • Verify that height is correctly changed when pasting code
  • Verify that the lines are correctly read and selection is correctly handled with screen readers
  • Check that when revealing a line, the scroll position is correct
    🐛 Because different line heights, the cursor is placed behind the sticky widget in certain cases. In our reveal logic we add as a padding top maxStickyLines * lineHeight but in our case line height can be variable.

To discuss

Diffs are not correctly aligned in a diff editor on either side when special line heghts are set ever since we have made line height define per editor and not per text model. We can see it in the following case:

Screenshot 2025-01-13 at 16 17 23

In the right editor the line heights are larger than the in left editor hence the diffs are no longer aligned. To mitigate this we have several options:

  1. The end user setting the line heights needs to set the line heights on the diff editor too
  2. We have to enforce editors in a diff to have the same line heights even if the user did not set this
  3. Don't allow changing the line heights inside of diff editors
  4. We make all editors set the same line height for a specific text model

As well as this, since the line heights are set per editor, from the diff editor, when you click on Toggle Collpased Unchaned Regions, this opens a diff editor where the line heights are the default ones. This is inconsistent user experience.

Issue discovered that should be fixed separately:

When revealing a line for example by using the cursor up comman, the cursor can end up below the sticky scroll widget which has the modified line height. This happens because we set as the padding: maxNumberOfStickyLines * lineHeight. In our case however lineHeight can be larger than the default line height, and so the padding is not enough, hence the cursor appears below the sticky widget. To fix this, I will have to redefine how view lines interact with the sticky scroll widget.

Decorations may affect line wrapping. For example, they make make the
text bold, or increase or decrease font size. This wasn’t originally
taken into account to calculate the the break offsets.
@aiday-mar aiday-mar self-assigned this Nov 5, 2024
@aiday-mar aiday-mar marked this pull request as draft January 10, 2025 09:34
auto-merge was automatically disabled January 10, 2025 09:34

Pull request was converted to draft

@aiday-mar
Copy link
Contributor Author

I have made a testing pass on the work in this PR which has revealed certain bugs which I need to address. The bugs and the findings are written in the PR summary at the top.

@aiday-mar aiday-mar requested a review from alexdima January 20, 2025 08:26
@aiday-mar aiday-mar marked this pull request as ready for review January 20, 2025 08:26
@aiday-mar aiday-mar modified the milestones: January 2025, February 2025 Jan 27, 2025
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.

Support variable line heights in the editor
4 participants