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

Hardcoded continuation prompt implementation with continuation_prompt feature #3

Draft
wants to merge 13 commits into
base: highlight
Choose a base branch
from

Conversation

zao111222333
Copy link

@zao111222333 zao111222333 commented Sep 16, 2024

Hi, I believe this version is well-defined, pls take a while to review:)

  1. Hardcoded continuation prompt implementation with continuation_prompt feature.
    See Refactor highlighting kkawakam/rustyline#795
    With example at examples/continuation_prompt.rs:
    demo1
    Also, verify with my python REPL project https://github.com/zao111222333/pyapp
    demo2

  2. Reduce tokenizer/parser overhead
    I believe the Helper in Editor should be helper: H, rather than helper: Option<H>, then we can avoid lifetime issue when using &mut self in highlight, validate, complete, ...
    as you have mentioned at
    https://github.com/gwenn/rustyline/blob/highlight/src/completion.rs#L93
    That's a quite large breaking change, but with this change, we can reduce the overhead and only tokenizer/parser once! see:
    before https://github.com/gwenn/rustyline/blob/highlight/src/lib.rs#L543
    now https://github.com/zao111222333/rustyline/blob/highlight/src/lib.rs#L547-L576

@zao111222333
Copy link
Author

The feedback is wanted, and I'd like to change along your suggestion :)

@gwenn
Copy link
Owner

gwenn commented Sep 30, 2024

See https://github.com/gwenn/rustyline/pull/2/files#diff-21d5a115156e525615d2209606c575558d8ba3348ad5d75bc36db7f06d3adfea
These methods cannot work anymore because of the continuation prompt...

@zao111222333
Copy link
Author

I see... How about this approach:
To support continuation prompt and scrolling, still use the Interator<Item = impl 'l+StyledBlock>, and count/check the \n str from StyledBlock::text() during writing to linebuffer.

The scrolling should be, user still highlight all of the lines, and rustyline will count the line number and decide the range of lines to render.

The continuation prompt should be, one rustyline find \n within the StyledBlock during writing linebuffer, rustyline will call highlight_continuation_prompt to write a continuation prompt after \n, and then countinue to render the rest of StyledBlock::text.

If this approach looks good to you, I can help.

@gwenn
Copy link
Owner

gwenn commented Oct 2, 2024

@gwenn
Copy link
Owner

gwenn commented Oct 2, 2024

Another issue is that currently the only repaint optimisation is here:
https://github.com/kkawakam/rustyline/blob/ceafb7f8057974201d0e76ac9fc7aaac3998a048/src/edit.rs#L364
(check that we can simply print on the screen the character which corresponds to the key pressed by the user instead of refreshing all line(s))
which is partially based on the fact that the Helper / Highlighter may be optional...
So when you say:

I believe the Helper in Editor should be helper: H, rather than helper: Option

I am sceptical

@zao111222333
Copy link
Author

zao111222333 commented Oct 3, 2024

So you reject / don't like the idea of having a layout cache https://github.com/gwenn/rustyline/pull/2/files#diff-7937a85ae16def66de428ea5dc339277fc8ca7bb7e40c247dc0b485acc39da6cR51 ?

I am looking into your layout cache. Can could you give me a little more information of pub type Cell = (usize, Position);? I guess a Cell represnets a byte, the Vec<Cell> represents all the input str with linebuff<->positon mapping, but I'm not sure.
And I can try to complete it when I understand your intention.

@zao111222333
Copy link
Author

zao111222333 commented Oct 3, 2024

which is partially based on the fact that the Helper / Highlighter may be optional...

I'm sorry but I am still a little confuse of the meaning of optional Helper / Highlighter.
I think the user will not set helper multi-times after the editor is create.
So how about let user specify / set the helper in Editor::new<H: Helper>(helper: H), for default helper, just input () as helper.
With this change we can avoid many lifetime issues, to let user can change helper itself in highlight, validate, complete, ...

@gwenn
Copy link
Owner

gwenn commented Oct 3, 2024

I'm sorry but I am still a little confuse of the meaning of optional Helper / Highlighter. I think the user will not set helper multi-times after the editor is create. So how about let user specify / set the helper in Editor::new<H: Helper>(helper: H), for default helper, just input () as helper.

If you think of rustyline as a enhanced Rust port of linenoise, you should understand that Helper is optional.
See https://github.com/antirez/linenoise/blob/d895173d679be70bcd8b23041fff3e458e1a3506/linenoise.h#L91-L92

And even if we could remove the setter, why not keep the Helper optional in constructor ?

Editor::new<H: Helper>(helper: Option<H>)

Also:
https://docs.rs/reedline/0.35.0/reedline/struct.Reedline.html#method.with_highlighter
(which means that it is optional)
Counterexample:
https://docs.rs/termwiz/0.22.0/termwiz/lineedit/struct.LineEditor.html#method.read_line
(but that means missing optimisations)

@zao111222333
Copy link
Author

zao111222333 commented Oct 4, 2024

Thank you for providing me those information. I known linenoise a bit when I modified it to implement python REPL in C.
Here I firstly want to make helper itself mutable in fn highlight, fn validate, fn complete..., merge it into highlight branch, since this change is stablized (in my opinion), and this change will influence the following commits.
Pls have a loot at #4

Then I will try to combine #2 and highlight.

@zao111222333 zao111222333 marked this pull request as draft October 4, 2024 08:11
@gwenn
Copy link
Owner

gwenn commented Oct 4, 2024

If possible, try to send a dedicated PR for mutable helper based on rustyline master branch (without highlight split stuff).

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.

2 participants