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

Samples on windows are inefficient #41

Open
pm100 opened this issue Oct 30, 2023 · 14 comments
Open

Samples on windows are inefficient #41

pm100 opened this issue Oct 30, 2023 · 14 comments

Comments

@pm100
Copy link
Contributor

pm100 commented Oct 30, 2023

On windows every key stroke is doubled due to the key press and release. TTA maps the release to Null but that null is still sent through the main input and draw loop. No harm is done but the key lookup and , more importantly, the render is repeated, uselessly.

I know these are samples but people will copy them.

There is no simple one liner, this isnt enough

        match crossterm::event::read()?.into() {
            Input { key: Key::Esc, .. } => break,
            Input { key: Key::Null, .. } => continue,

as all the samples do render then input, so the render gets done again

@rhysd

This comment was marked as outdated.

@rhysd

This comment was marked as outdated.

@rhysd rhysd closed this as completed Nov 1, 2023
@rhysd rhysd reopened this Nov 1, 2023
@rhysd
Copy link
Owner

rhysd commented Nov 1, 2023

Oh, sorry, I didn't get your point. So you suggest some improvement in example.

@rhysd
Copy link
Owner

rhysd commented Nov 1, 2023

But I'm not sure this is a problem specific to this crate. All widgets have the same issue, maybe? It is not possible to know when to render the widget only from key input. Demo example of ratatui has the same issue. It calls Terminal::draw twice on press and release:

https://github.com/ratatui-org/ratatui/blob/9f371000968044e09545d66068c4ed4ea4b35d8a/examples/demo/crossterm.rs#L50-L75

I think this is an issue of ratatui's API design rather than handling it within this crate somehow. What we can do is rendering the first screen separately like this:

// First rendering
term.draw(|f| {
    f.render_widget(textarea.widget(), f.size());
})?;

loop {
    match crossterm::event::read()?.into() {
        Input { key: Key::Esc, .. } => break,
        Input { key: Key::Null, .. } => continue,
        input => {
            textarea.input(input);
        }
    }
    term.draw(|f| {
        f.render_widget(textarea.widget(), f.size());
    })?;
}

@rhysd
Copy link
Owner

rhysd commented Nov 1, 2023

If my understanding is correct, ratatui checks diff of content and updates terminal screen differentially. So, even if the draw method call is doubled, it may not be a big performance issue. Paragraph widget is created, ratatui checks if there is difference, and it simply discards the widget since there is no difference.

@joshka
Copy link
Contributor

joshka commented Nov 1, 2023

This is less ratatui design, and more that most of the examples use this common approach (the loop { read, draw} isn't the only way to do things) . I think most of the examples are wrong in this, but wrong in a way that doesn't usually cause any problem. An application main loop should probably not just re-render on every keystroke (thought exercise - If I type at 100WPM and 5 chars + space per word => 0.1ms per char then typing adds 10fps to the necessary tick rate just to screen updates to handle typing). I prefer the async approach with a 60fps interval (or the manual non-async equivalent of not re-drawing more than ~60fps)

@pm100
Copy link
Contributor Author

pm100 commented Nov 1, 2023

no saying its slow just that its ugly to see the input/render cycle run twice for every key stroke. Rust is supposed to be super fast, hate to see cycles wasted

@rhysd
Copy link
Owner

rhysd commented Nov 2, 2023

I've thought about this for a while. But I think this issue cannot be resolved generally. It's kind of optimization specific to application.

Key::Null means that this key is not supported by tui-textarea. However, it does not mean it is not supported by other widgets which are rendered in the same screen. Other widgets may do some action on the key input. It may eventually change the state of tui-textarea's widget. For example, some widget may change its size and it may affect the size of viewport of textarea. It means, even if the input was Key::Null, the textarea needs to re-render the content since the viewport was changed.

We can ignore the press event safely on minimal example since we know that the textarea is the only widget rendered on the screen. However the technique is specific to the example and cannot be applied to other applications generally.

It would be nice to have some performance tips in document so that users can consider to apply the technique to their applications.

@joshka You're right. But PowerShell's rendering is very slow. So I'm skeptical that it can meets the 10fps requirement on Windows honestly.

@pm100
Copy link
Contributor Author

pm100 commented Nov 2, 2023

its just the samples. It cannot be fixed in general. I fixed the error in gitui (the project I am using textarea for, they had a much more serious issue, they were processing the release as a key stroke so everything was duplicated !). But the samples are meant to show how to correctly use the API.

THis fix works in minimal, can be generalized for all samples.

    loop {
        term.draw(|f| {
            f.render_widget(textarea.widget(), f.size());
        })?;
        let mut key: Input;
        loop {
            key = crossterm::event::read()?.into();
            if key.key != Key::Null {
                break;
            }
        }
        match key {
            Input { key: Key::Esc, .. } => break,
            input => {
                textarea.input(input);
            }
       

@rhysd
Copy link
Owner

rhysd commented Nov 2, 2023

THis fix works in minimal, can be generalized for all samples.

For example, your code doesn't work properly when you resize the terminal window.

@rhysd
Copy link
Owner

rhysd commented Nov 2, 2023

Your wrong assumption is that receiving Key::Null input does not change the rendering state of textarea widget. It actually does.

@pm100
Copy link
Contributor Author

pm100 commented Nov 2, 2023

ah yes, so really key into should return Key::Skip or something to indicate that this input can be ignored.

@rhysd
Copy link
Owner

rhysd commented Nov 5, 2023

You mean converting key events whose kind is Release into Key::Skip or Key::Ignored or something, right? It would work in almost all cases but not in 100% cases. Some other widget may catch the Release event and change its state. It may eventually change the state of textarea widget (e.g. resize due to the layout change). Ignoring any input event cannot be done generally.

If you know all other widgets in your application ignore Release events, you can safely do the following even if we don't add Key::Skip thing.

    use crossterm::event::{Event, KeyEvent, KeyEventKind};

    term.draw(|f| {
        let rect = ...; // Get area to render textarea
        f.render_widget(textarea.widget(), rect);

        // Render other widgets...
    })?;
    loop {
        let event = crossterm::event::read()?;
        if let Event::Key(KeyEvent { kind: KeyEventKind::Release, .. }) = &event {
            continue;
        }
        match event.into() {
            Input { key: Key::Esc, .. } => break,
            input => {
                textarea.input(input);
            }
        }
        term.draw(|f| {
            let rect = ...; // Get area to render textarea
            f.render_widget(textarea.widget(), rect);

            // Render other widgets...
        })?;
    }

@pm100
Copy link
Contributor Author

pm100 commented Nov 5, 2023

I know how to deal with it in real life - I am a user of textarea in another project , the double keystrokes from crossterm were a huge issue. I am just trying to see if a) it needs to be documented b) we can fix the samples.

Maybe do nothing. I just hursts my brain whenever I put traces in the code and see everything triggered twice

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

No branches or pull requests

3 participants