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

Rework text rendering and integrate softwrap rendering #5008

Closed
wants to merge 3 commits into from

Conversation

pascalkuthe
Copy link
Member

@pascalkuthe pascalkuthe commented Dec 5, 2022

This PR implements a general rework of the text rendering system.
The main aim is to make the text rendering more flexible to easily allow implementation of various decorations or
even new text-based views besides editor without hard-coding them into text rendering.
It also contains an implementation of soft-wrapping rendering (not actually usable right now see below).

While working on this PR I had the following features in mind:

Text Render System Desgin

This PR completely replaces the render_text_highlights function (but leaves the rest of the editor mostly untouched).
Although this new implementation looks quite different as it's structured it's not a complete rewrite and I heavily leaned on the old implementation.

My goal during the implementation was primarily separating different components of the text rendering.
The text rendering is now roughly split into three basic components:

  • Basic grapheme rendering and visual position tracking (TextRender)
  • Transversing the document efficiently and accumulating graphemes for rendering (DocumentCursor)
  • Gluecode that handles syntax highlighting, soft-wrapping and integration of inline decorations (DocumentRender)

The primary interaction with the text rendering happens through the DocumentRender struct.
It allows the caller to render a single visual line (which could just be a part of a full line in case of soft-wrapping)
with the reder_line function.
It behaves similar to a rust iterator (lazy) so it only renders a line if this function is called.
This function requires a mutable reference to TextRender which tracks the actual onscreen position.

Thanks to this decoupling TextRender can be used to render other text in between lines.
For example a second DocumentRender could be used to interpose the removed lines of a second document (with syntax highlighting and soft-wrapping)
at hunks for a unified diff view.

Furthermore, DocumentRender yields after each visual line (instead of only after line break) to ensure other rendering components
can take soft wrapping into amount.
As the DocumentCursor/DocumentRender tracks the position within the document (both line/col and total char_offset/byte_offset) anyway
this can be used to track the exact beginning/end of wrapped lines (so the diagnostic gutter could be shown on the correct soft-wrapped line).
This information can also be used by custom views to interrupt the rendering to render other text with TextRender at line bounds.
In this PR I simply used this property to ensure gutters are displayed in the correct place when soft-wrapping is enabled.

Inline annotations/overlays can easily be integrated with a trait that would be called from within the push_grapheme function.

Performance

Compared to the old text rendering the document transversal works very different now.
Previously RopeSlices were created for each syntax range text.slice(highlight_start..highlight_end).
For these slices a RopeGraphemes iterator was then created and rendered in a single continuous (non-interruptible) loop.

This approach does not work well for this PR because we need the iteration to be interruptible.
Therefore, the graphemes are collected into a temporary vector instead which is rendered at:

  • Word boundaries
  • Line breaks
  • at the end of the visual line when soft-wrapping is enabled
  • after a fixed chunk size when soft-wrapping is disabled (to avoid excessive memory consumption)

The overhead should be minimal as the buffer never exceeds a fixed size (viewport width with soft-warping, 64 without).

As both the slice() function and the creation of RopeGraphemes are log(N) operations.
The code now uses a single RopeGraphemes iterator for the entire text (so no more slicing at highlight boundaries)
which should be a nice performance boost.
Another advantage of this approach is that text rendering can now easily be refactored to use byte positions instead of char
positions (which avoids the costly NlogN char -> byte conversion currently performed for treesitter queries).
However, I left this as a followup PR for now as it requires some code outside the rendering system.

Soft Wrapping

The PR implements soft-wrapping this is partly inspired by led (credit to @cessen here).
The text is wraps words that would exceed the line-width at their starting boundary (spaces) when possible.
When the text that would need to be wrapped exceeds a certain (configurable) width (5 by default) the word start
is instead broken exactly at the line end.
In practice this usually wraps text well while only causing minimal overhead (there are no pathological cases with this approach).

Right now only (breaking) spaces (so tabs and space) are treated as word boundaries.
To better wrap source code I am considering treating everything that is not char::is_alphanumeric as a word boundary instead.
This would wrap long paths in rust at . for example.

Wrapped lines are indented with a configurable amount of space (2 by default).
They furthermore retain the indentation (and indent guides) from the start of the line unless
this indentation exceeds as (configurable) limit (people that don't like this feature can set the limit to 0).

Landing this

The rendering system in this PR works quite well, although more testing is obviously needed.
Rendering soft-wrapping behaves well and has all the features I personally except.
However, soft-wrapping is currently not handled well in the rest of the editor which excepts that.
View::offset corresponds to document lines/columns which is not the case anymore with this PR.
To address this we need some.way to find wrapped line starts which is why I would probably move.some.of the softwrapping code into DocumentCursor which could then be moved to helic-core to provide that functionality everywhere in helix.

Gutters also need further improvements (for example insert and modify diff gutters should be shown for all wrapped lines).

In interest of landing this PR faster and enable work on orthogonal features (like side-by-side diffs)
that could leverage the improved rendering system I propose to land this PR as is (with some minor improvements)
and disable soft-wrapping with a (off by default) feature flag (everywhere config.soft_wrap.enable is used simply add && cfg!(feature=softwrap)).

This PR still has some rough edges to polish:

  • Clean up code documentation/comments
  • Maybe some tests, although I am not sure how to actually test this yet (maybe asserting the contest of a surface after render?)
  • Add the feature flag mentioned above
  • Adjust user facing documentation (better documentation for the soft wrap config options with a note that this is an in-development feature that is disabled)
  • undo some changes to view positioning that were added to demo text wrapping

A big shoutout to @kirawi for reaching out to me about potential overlap between virtual text.
The initial design discussions with him heavily influenced this PR.

@pascalkuthe pascalkuthe added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Dec 5, 2022
@kirawi kirawi mentioned this pull request Dec 5, 2022
Comment on lines 218 to 242
if let Some((row, col)) = self.offset_coords_to_in_view(doc, scrolloff) {
if let Some((row, _col)) = self.offset_coords_to_in_view(doc, scrolloff) {
self.offset.row = row;
self.offset.col = col;
self.offset.col = 0;
Copy link
Member Author

@pascalkuthe pascalkuthe Dec 5, 2022

Choose a reason for hiding this comment

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

Only included so soft-wrapping can be tried out, will be removed before merging (altough the basic idea of avoiding horizontal view movement when softwrapping is enabled is correct)

Comment on lines 82 to 88
// pub fn byte_pos(&self) -> usize {
// self.graphemes.byte_pos()
// }

// pub fn char_pos(&self) -> usize {
// self.char_pos
// }
Copy link
Member Author

Choose a reason for hiding this comment

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

I want to keep these functions for use in future work but they are currently not used. and hence generate warnings.

@archseer
Copy link
Member

archseer commented Dec 5, 2022

To update all the view methods with calculations that take wrapping and widgets into account we might want to design something similar to Xi's line cache:

https://github.com/xi-editor/xi-editor/blob/master/rust/core-lib/src/width_cache.rs
https://github.com/xi-editor/xi-editor/blob/master/rust/core-lib/src/line_cache_shadow.rs
https://github.com/xi-editor/xi-editor/blob/master/rust/core-lib/src/linewrap.rs

They did async line wrapping which got pretty complicated (https://raphlinus.github.io/xi/2020/06/27/xi-retrospective.html#async-is-a-complexity-multiplier) but at least it might give you some ideas. You can then query the cache for measurements to figure out visual offsets

@archseer
Copy link
Member

archseer commented Dec 5, 2022

Maybe some tests, although I am not sure how to actually test this yet (maybe asserting the contest of a surface after render?)

tui came with a TestBackend (https://github.com/helix-editor/helix/blob/master/helix-tui/src/backend/test.rs) that we should enable in e2e tests (currently we compile the editor without the render function instead). Then you'd be able to do asserts based on screen contents

@archseer
Copy link
Member

archseer commented Dec 5, 2022

Basic grapheme rendering and visual position tracking (TextRender)
Transversing the document efficiently and accumulating graphemes for rendering (DocumentCursor)
Gluecode that handles syntax highlighting, soft-wrapping and integration of inline decorations (DocumentRender)

Let's add these as doc comments to the types

Space,
Nbsp,
Tab,
Other { raw: Cow<'a, str>, width: u16 },
Copy link
Member

@kirawi kirawi Dec 5, 2022

Choose a reason for hiding this comment

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

Is it worth storing the width as a field? unicode-width uses a lookup table so it should be O(1).

Copy link
Member

Choose a reason for hiding this comment

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

It's a bit more expensive than that, it has a few layers of subtables (0,1,2) + it has to unpack the subtable

Copy link
Member Author

Choose a reason for hiding this comment

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

As @archseer mentioned it's not quite free and the cost of storing the u16 here is not that large. In fact due to alignment (Cow is aligned to 8 bytes) it might actually be free as the size of the enum might not change at all (didn't check tought). Note that if we cared about the size of this enum enough for that to matter using beef to get a faster/smaller cow would be more worthwhile then saving the width. It also doesn't introduce any complexity in the code (it reduces it in fact) so I don't see a downside.

@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Dec 8, 2022
@matoous matoous mentioned this pull request Dec 8, 2022
8 tasks
@blitzerr
Copy link

@pascalkuthe thanks a lot for this PR. This is one of the most desirable features. I am very exited about this hope this gets merged soon. Let me know if there is something I can do to help.

@xJonathanLEI
Copy link
Contributor

@blitzerr For starter, help test the PR by daily driving with it? That's the easiest way to help get the PR across the finish line.

@xJonathanLEI
Copy link
Contributor

Not sure if this PR is ready to receive bug reports given its draft status, but here is one anyways. I'm using current PR head (81fa643) rebased on master and fixes for the build errors.

There problem is there are unexpected view shifts when the cursor is on at least the 3rd visual line of soft-wrapped lines.

To illustrate the issue, first make sure you're view width is small. I used 100 when testing this:

$ tput cols
100

Then open the editor with this long text:

$ curl -L https://github.com/helix-editor/helix/files/10274419/test.txt | hx -c /dev/null

With the editor open, press gl to go to the line end.

Expected result: the view itself is not moved and the cursor position is the only thing changed.

Actual result: the view is shifted downwards.

Perhaps this is best illustrated with a asciinema recording:

asciicast

@gabydd
Copy link
Member

gabydd commented Dec 21, 2022

was looking at your error and I found an interseting one of my own:

  1. enter a scratch buffer and set the language to markdown
  2. insert # <and a continuous amount of the letter a>
  3. at some point it will panic with:
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `81`,
 right: `1`: Highlight scope must be aligned to grapheme boundary', /home/gaby/dev/helix/helix-core/src/doc_cursor.rs:313:17

with the relevant backtrace

2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
             at /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/core/src/panicking.rs:182:5
   4: helix_core::doc_cursor::DocumentCursor<S,A>::advance_with_highlight
             at ./helix-core/src/doc_cursor.rs:313:17
   5: helix_term::ui::document::DocumentRender<A,H>::render_line
             at ./helix-term/src/ui/document.rs:107:40
   6: helix_term::ui::editor::EditorView::render_text_highlights
             at ./helix-term/src/ui/editor.rs:478:13
   7: helix_term::ui::editor::EditorView::render_view
             at ./helix-term/src/ui/editor.rs:158:27
   8: <helix_term::ui::editor::EditorView as helix_term::compositor::Component>::render
             at ./helix-term/src/ui/editor.rs:1350:13
  1. the left value doesn't seem to be random if you do tput cols it is 6 less then that
  2. it can only be reproduced when the text traversed is being highlighted
    I am going to try and do some more digging tomorrow but I wanted to keep track of the issue,

(also for the above issue ensure cursor in view isn't changed to work with this pr yet so that might be part of the problem I am not sure, making this all behaves well with more then two lines of softwrap does seems important for the future though)

@pascalkuthe
Copy link
Member Author

Thank you for the bug reports. I am mostly aware of these limitations already, these will be fixed once I finish working on this.

This branch is not yet ready for daily driving and I know its both limited and buggy. Further changes are necessary and there is even some design work left to do.

In fact the latest push was mostly to get my local dev state into github so I can work on it on my laptop (hence the all-red CI) while I visit my family during the holidays. But I likely won't have time to finish this PR until new year.

@pascalkuthe
Copy link
Member Author

While further working on this PR the implementation changed enough that I felt it was more appropriate to create a new PR rather than to push to this branch. Closed in favor of #5420

@pascalkuthe pascalkuthe closed this Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants