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.

Sign up for GitHub

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

Jump to bottom

Revert resize handling removal #478

Closed
wants to merge 3 commits into from
Closed

Revert resize handling removal #478

wants to merge 3 commits into from

Conversation

sholderbach
Copy link
Member

Attempt to test if bringing the basic resize behavior back will improve the situation in some terminals that implicitly resize on startup

Tries to fix #477

@sholderbach sholderbach added the A-Display Area: Correctness of the display output and additional features needed there label Sep 12, 2022
@fdncred
Copy link
Collaborator

fdncred commented Sep 13, 2022

i just check this out and built it and have the animated clock. I'm so confused.

@sholderbach
Copy link
Member Author

Sorry my bad, I got the logic mentally backwards.

@dbuch
Copy link

dbuch commented Sep 18, 2022

I've tested this, and i solves the right prompt problem. I vote get it in, and progress from here.
There are some issues still that are annotated by todo/fix in code:

https://github.com/nushell/reedline/blob/main/src/painting/painter.rs#L383
https://github.com/nushell/reedline/blob/main/src/painting/painter.rs#L387

These probably needs attention :) I did try look into it. Logging/tracing would be useful I think @sholderbach, @fdncred would that be desired i could cook somthing up?

@fdncred
Copy link
Collaborator

fdncred commented Sep 18, 2022

@dbuch oh, please do look at this. I'd love to land this but it's really trading one problem for another. however, if I knew you planned to work on the remaining issues I personally would feel much more comfortable landing this one with the hopes that you could cook something up to make it better. note, I have no fantasy that you can or would fix everything but we generally approve iterative approaches to solving "hard" issues like this. So, yes, please cook something up. :)

@dbuch
Copy link

dbuch commented Sep 18, 2022

@fdncred Okay, I'll try. I still think this piece should be merged, do you know what the trade are?
This PR is after all - afaik - fairly okay now that amimation isn't restored again.

Next step, for me atleast, would be to implement some logging preferably to some file.

@fdncred
Copy link
Collaborator

fdncred commented Sep 18, 2022

@dbuch I think if you run this PR and resize left and right and diagonally left and right, you'll see artifacts. Now if you do the same thing before this PR, there are probably less artifacts with resizing. However the tradeoff with before this PR is that sometimes you start nu and you get a prompt at the top of the screen, hit enter and all of a sudden the prompt is in the center of the screen.

@dbuch
Copy link

dbuch commented Sep 18, 2022

@fdncred you are right! I use kitty, and because (i think) i use HDPI and kitty resizes late i get that prompt in the middle. But only at the beginning.
But the cause of this is likely because of invalid row calculation when shrinking down. I'll investigate abit more.

@sholderbach
Copy link
Member Author

A fresh set of eyes looking at this problem is absolutely welcome! @dbuch

In my experimentation the resizing artifacts were especially pronounced when there was content close to the bottom of the screen + wrapping of text. Also the choice of terminal emulator had some influence on the severity of the visual artifacts. While kitty for example seems to hold of until sending the resize event, alacritty seemed to eagerly send events that were not properly handled by reedline.

Having optional tracing to study what is happening would be great, especially maybe with timing information to tweak how our event loop reacts to those things (

reedline/src/engine.rs

Lines 40 to 50 in 7d721a1

// The POLL_WAIT is used to specify for how long the POLL should wait for
// events, to accelerate the handling of paste or compound resize events. Having
// a POLL_WAIT of zero means that every single event is treated as soon as it
// arrives. This doesn't allow for the possibility of more than 1 event
// happening at the same time.
const POLL_WAIT: u64 = 10;
// Since a paste event is multiple Event::Key events happening at the same time, we specify
// how many events should be in the crossterm_events vector before it is considered
// a paste. 10 events in 10 milliseconds is conservative enough (unlikely somebody
// will type more than 10 characters in 10 milliseconds)
const EVENTS_THRESHOLD: usize = 10;
,

reedline/src/engine.rs

Lines 484 to 499 in 7d721a1

if event::poll(Duration::from_millis(100))? {
let mut latest_resize = None;
// There could be multiple events queued up!
// pasting text, resizes, blocking this thread (e.g. during debugging)
// We should be able to handle all of them as quickly as possible without causing unnecessary output steps.
while event::poll(Duration::from_millis(POLL_WAIT))? {
match event::read()? {
Event::Resize(x, y) => {
latest_resize = Some((x, y));
}
enter @ Event::Key(KeyEvent {
code: KeyCode::Enter,
modifiers: KeyModifiers::NONE,
}) => {
crossterm_events.push(enter);
).
My memory is failing me what problem I was trying to observe but in one case I couldn't reproduce a certain behavior while launching reedline from lldb.

@sholderbach
Copy link
Member Author

PR has no direct connection to current problems or codebase. Closing despite some interesting discussion

@sholderbach sholderbach closed this Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-Display Area: Correctness of the display output and additional features needed there
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nu doesn't handle resizing intelligently
3 participants
@sholderbach @fdncred @dbuch