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

Keep track of layout of input buffer spans #820

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aljazerzen
Copy link

Builds on top of https://github.com/gwenn/rustyline/pull/2/files

When moving cursor up or down a line, it should not change columns. To be precise, it should not change columns on the screen.

Current implementation was making sure it was not changing number of graphemes from the beginning of the line. This works correctly, as long as prompt is empty. If it so not empty, the cursor moves up and down like this:

> c_mmand \
argument1
> command \
a_gument1

This PR changes this behavior to this:

> c_mmand \
argument1
> command \
arg_ment1

This change is also needed for continuation prompts.


To make this happen, I had to:

  • keep track of layout of input buffer spans (one for each line in the input buffer),
  • refactor Renderer::compute_layout into Layout::compute,
  • change LineBuffer::move_to_line to take Layout as a parameter,
  • fix State::move_cursor.

@aljazerzen aljazerzen mentioned this pull request Oct 22, 2024
3 tasks
@gwenn
Copy link
Collaborator

gwenn commented Oct 22, 2024

Sorry, I may have missed something but I don't see:

We want something that can tell to the LineBuffer, the (line, column) associated to one byte / grapheme offset and vice-versa.
(#819 (comment))

So you need to recompute almost everything.
I may be wrong but it seems possible (and mandatory for continuation prompt / scrolling / ...) to have a layout cache (and its minimal invalidation) which can provide this information: (line, column) <=> byte / grapheme offset.

@aljazerzen
Copy link
Author

The Layout is this layout cache:

  • to get offset from (row, column), you lookup span by row and get the start offset of the span. Then one has to count column graphemes from that offset in the LineBuffer to get the final offset,
  • to get (row, column) from offset, you lookup span by offset and then run a renderer.calculate_position(&buf[span.offset..offset], span.pos) to get the final (row, column).

Is this what you are asking about?

@gwenn
Copy link
Collaborator

gwenn commented Oct 23, 2024

The Layout is this layout cache:

  • to get offset from (row, column), you lookup span by row and get the start offset of the span. Then one has to count column graphemes from that offset in the LineBuffer to get the final offset,
  • to get (row, column) from offset, you lookup span by offset and then run a renderer.calculate_position(&buf[span.offset..offset], span.pos) to get the final (row, column).

Is this what you are asking about?

Without the renderer.calculate_position, yes !

So you need to recompute almost everything.
(#820 (comment))

@aljazerzen
Copy link
Author

Without the renderer.calculate_position, yes !

Well, this call is only needed for getting positions within a span. If one wants to know where does a line start, no calculate_position is needed.

@aljazerzen
Copy link
Author

In any case, this PR solves a part of the problem you are describing and is useful in its own.

@aljazerzen
Copy link
Author

Also, where would this offset <-> Position mapping be needed?

@gwenn
Copy link
Collaborator

gwenn commented Oct 23, 2024

Also, where would this offset <-> Position mapping be needed?

When you move up or down like you already know, for scrolling vertically (when screen height < number of rows) (not supported currently), to repaint only edited / impacted line(s) (not supported currently), ...

https://github.com/gwenn/rustyline-notes/blob/master/src/layout.md

@gwenn
Copy link
Collaborator

gwenn commented Oct 23, 2024

See also https://github.com/daanx/isocline/blob/c9310ae58941559d761fe5d2dd2713d245f18da6/src/stringbuf.h#L88-L93
and their usages / call hierarchy.
There is no cache but these two methods are used almost everywhere (mostly the second one sbuf_get_rc_at_pos).

@aljazerzen
Copy link
Author

What's the status of this PR now? Can it be merged?

I do believe it has merit of its own, even though it does not implement layout caching or selective refresh.

@gwenn
Copy link
Collaborator

gwenn commented Oct 27, 2024

If you don't mind, I would prefer this simple patch: #822
And then have a proper layout cache.

@aljazerzen
Copy link
Author

Ok, but then I must also add a prompt_continuation_size to layout, right?

@gwenn
Copy link
Collaborator

gwenn commented Oct 28, 2024

Ok, but then I must also add a prompt_continuation_size to layout, right?

For continuation_prompt, we need more than that...
See #795.
And we might not need neither prompt_continuation_size nor prompt_size for movement if we have our offset <-> Position mapping...

@aljazerzen
Copy link
Author

For continuation_prompt, we need more than that...

I don't think we do.

See #795.

What am I looking for?

... if we have our offset <-> Position mapping

This PR adds this mapping! It contains enough information to not make any calls to calculate_position during redrawing. If we add caching, it would also eliminate calls to calculate_position in most LineBuffer updates. My illustration of how to map offset to position and back calls calculate_position only for offsets which don't fall on the start of any line.

@gwenn
Copy link
Collaborator

gwenn commented Oct 29, 2024

I don't think we do.

Ok, so you think that we can support continuation prompt without fixing highlighting.
https://github.com/gwenn/rustyline-notes/blob/master/src/design.md#highlighting

Currently, the Highlighter returns directly a styled (ansi escape sequence) text for the whole input lines. But for continuation prompt or scrolling we need to split the input by line (soft wrapped or not).

It contains enough information to not make any calls to calculate_position during redrawing.

This line
https://github.com/aljazerzen/rustyline/blob/30e25bf3413b9bf786bdea434182efa7562349fa/src/edit.rs#L176
proves the contrary.

If we add caching

How are you going to invalidate / refresh the cache ?
(I am pretty sure that you cannot partially invalidate / refresh the cache without offset <-> Position mapping)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants