Visible whitespace only for selections

[tomKPZ]

No description provided.

[tomKPZ]

This is my first foray into rust so the code may not be idomatic

[the-mikedavis]

Sorry for the delay on this. Admittedly it took me a while to figure out how this was working despite being familiar with these parts of the codebase 😅

[the-mikedavis]

I'm not sure this makes sense or if it's too much of a micro optimization but we could not emit selection events when none of the WhitespaceRenderValues are for select. What do you think?

[tomKPZ]

I'm not sure this makes sense or if it's too much of a micro optimization but we could not emit selection events when none of the WhitespaceRenderValues are for select. What do you think?

Sure, done in latest revision

[archseer]

Sorry it took a while! I wonder if we could avoid additional event types and instead just track this in the render loop. When HighlightStart(selection scope) is emitted we'd toggle a local bool to true, then false when it's popped off the scope stack

[tomKPZ]

Done. I hope I've understood your suggestion properly

[the-mikedavis]

I think archseer is saying that you can track this without changing the helix_core::syntax by looking up the scope for the highlight event and seeing if it's a selection scope:

HighlightEvent::HighlightStart(span) => {
    if theme
        .scopes()
        .get(span.0)
        .map(|scope| scope.starts_with("ui.selection"))
        .unwrap_default()
    {
        selecting = true;
    }
    spans.push(span);
}
// and for HighlightEnd, check that the popped scope starts with "ui.selection"

that snippet ☝️ isn't very performant though - we should calculate which indices in theme.scopes() correspond to selection highlights outside of the for

[tomKPZ]

I see, thanks for clarifying. I think I've done it in the latest revision

[the-mikedavis]

It looks like whitespace ends up not getting rendered for the cursor. For that we may need to include ui.cursor and ui.cursor.* (but there's also ui.cursorline so we can't use a starts-with check)

[archseer]

I've done some simplifications, this way we can avoid comparing spans against the set of selection scopes in most cases. I'm also thinking if we want to remove that render closure for a regular function

[tomKPZ]

Thanks for the commits @archseer !

I've added a fix for ui.cursor that @the-mikedavis found, and I've removed the inner render_whitespace closure, so no more dynamic dispatch is required for each whitespace char rendered.

[pascalkuthe]

With #5420 merged this PR will need to be ported to the new text rendering in ui/document.rs

[pascalkuthe]

clsoing this one as stale as the rendering code has been entirely replaced. Thank you for contributing