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

Optimized: Allowing to define variable line heights 2 #241218

Draft
wants to merge 86 commits into
base: main
Choose a base branch
from

Conversation

aiday-mar
Copy link
Contributor

@aiday-mar aiday-mar commented Feb 19, 2025

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 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
  • Verify that sticky scroll rendering is correct.
    • folding icon is correctly positioned
    • line height corresponds to the editor line height
  • 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.

@aiday-mar aiday-mar self-assigned this Feb 19, 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
3 participants