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

Implementation of scrollback #657

Closed
wants to merge 99 commits into
base: master
from

Conversation

Projects
None yet
@neon64

neon64 commented Jul 12, 2017

The Grid<T> is now a view into a region (I called it the 'active region') of a VecDeque. When new lines are added, they are pushed onto the back of the queue, and once the scrollback buffer is considered 'full', then old lines are popped off the front of the queue. This seems to work rather well since, unlike a Vec, elements don't need to be reshuffled around very often.

Of course, as discussed in #124 there are a few things that need to be fixed before I'd consider this 'ready':

  • configurable max scrollback length
  • write new tests
  • fix existing ref tests
  • measure performance impact (it seems fine but some numbers would be more reassuring)
  • fix out of bounds errors on resize
src/grid.rs Outdated
unsafe {
// check that src/dst are in bounds. Since index::Line newtypes usize,
// we can assume values are positive.
// we can implassume values are positive.

This comment has been minimized.

@tbodt

tbodt Jul 12, 2017

Contributor

wut

This comment has been minimized.

@neon64

neon64 Jul 12, 2017

Oh lol. That's obviously a typo

This comment has been minimized.

@neon64

neon64 Jul 15, 2017

I've fixed it in ac283f9

This comment has been minimized.

@OJFord

OJFord Jul 15, 2017

Contributor

No, I think it should enter the vernacular:

implassume (verb)
(im·pluh·syoom)

  1. To assume such is the case; implement it if it turns out not to be.
@nh2

This comment has been minimized.

nh2 commented Jul 19, 2017

When I use this branch in i3 (alone on a workspace), run dmesg in alacritty to get lots of output, and then full-screen and un-full-screen the window, alacritty crashes at the un-full-screen step with this stack trace:

$ env RUST_BACKTRACE=1 target/release/alacritty-scroll
Resizing Line(10) Column(101)
Resizing Line(10) Column(101)
Resizing Line(68) Column(177)
Resizing Line(68) Column(177)
Resizing Line(70) Column(177)
Resizing Line(70) Column(177)
Resizing Line(68) Column(177)
Resizing Line(68) Column(177)
thread 'main' panicked at 'Out of bounds access', /checkout/src/libcore/option.rs:794
stack backtrace:
   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
             at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::panicking::default_hook::{{closure}}
             at /checkout/src/libstd/sys_common/backtrace.rs:71
             at /checkout/src/libstd/sys_common/backtrace.rs:60
             at /checkout/src/libstd/panicking.rs:355
   2: std::panicking::rust_panic_with_hook
             at /checkout/src/libstd/panicking.rs:371
             at /checkout/src/libstd/panicking.rs:549
   3: std::panicking::begin_panic
             at /checkout/src/libstd/panicking.rs:511
   4: std::panicking::begin_panic_fmt
             at /checkout/src/libstd/panicking.rs:495
   5: core::panicking::panic_fmt
             at /checkout/src/libstd/panicking.rs:471
   6: core::option::expect_failed
             at /checkout/src/libcore/option.rs:794
   7: alacritty::run
             at /checkout/src/libcore/option.rs:297
   8: alacritty::main
             at ./src/main.rs:45
   9: main
  10: __libc_start_main
  11: _start

This does not happen on alacritty master commit 94849c4, so I suspect this PR introduces this crash.

@monouser7dig

This comment has been minimized.

monouser7dig commented Jul 19, 2017

actually works well on macOS

@ianks

This comment has been minimized.

ianks commented Jul 20, 2017

Tested it out for a bit on Linux (X not wayland). It is working incredibly well!

@neon64

This comment has been minimized.

neon64 commented Jul 21, 2017

Thanks @nh2 and @qxzw for the reports. I'm aware that resizing at the moment is really buggy on my fork - I have some improvements underway right now however it could be slow progress (for me it is during the school term time so I don't have much free time).

@brycefisher

This comment has been minimized.

Contributor

brycefisher commented Jul 27, 2017

@neon64 -- did you want to hand off this PR to another contributor?

@neon64

This comment has been minimized.

neon64 commented Jul 28, 2017

@brycefisher I'm really sorry about the delays with this - I'm happy to keep driving the PR forward, but it may be at a slower pace than others would like, so in that way I think its a great idea if others want to contribute as well.

@nh2

This comment has been minimized.

nh2 commented Jul 28, 2017

@neon64

I can confirm the pushed commit 1be37ff fixes the crashing problem.

But the commit on top 92a9d9b9 - Merged changes from upstream breaks scrolling (it simply doesn't scroll).

@jwilm

This comment has been minimized.

Owner

jwilm commented Jul 28, 2017

@neon64 this might be easier to maintain if you could rebase instead of merging master.

@neon64

This comment has been minimized.

neon64 commented Jul 28, 2017

@jwilm I hope I've rebased it correctly - it was a bit messy because I had to rewind the merge as well

@neon64

This comment has been minimized.

neon64 commented Jul 29, 2017

There's also a really subtle bug that I might need help diagnosing... If I run some command that just stays running with no output: eg tput -S or cat, and then resize the window, I get lots of visual glitches. I've tracked this down to the window not being redrawn on resize (usually it is because the shell prints a new prompt), however if I add self.dirty=true; inside Term::resize it doesn't help. I think this is because resizing is separate from the normal event loop, or at least doesn't follow the usual input->update->render cycle like eg: key input. I'm afraid I'm really unfamiliar with this part of the internals so any help would be appreciated.

@nh2

This comment has been minimized.

nh2 commented Jul 29, 2017

I hope I've rebased it correctly - it was a bit messy because I had to rewind the merge as well

@neon64 There seems to be somethin in the recent master that breaks the scrolling feature when scrolling with the ThinkPad "middle" button (which is my main way of scrolling).

On your rebased commit c531d78 I cannot scroll with the middle button, same as in the case where you merged. But it worked for me on your original commit 1be37ff.

So currently commit 1be37ff is the only one where I can scroll with both mouse and ThinkPad middle button, and it doesn't crash when using fullscreening.

@neon64

This comment has been minimized.

neon64 commented Jul 30, 2017

@nh2 I'm 99% sure that's got nothing to do with my code, but rather something on the latest version of glutin/winit that means that the 'on scroll' callback never gets called (or with different values). Try adding some debugging println!s inside of this function to see what's happening. Which platform are you testing this on?

@nh2

This comment has been minimized.

nh2 commented Jul 30, 2017

@neon64 Platform is Ubuntu 16.04.

Yes, I can confirm that on the working branch, on_mouse_wheel fires for both real mouse wheel and Thinkpad button. On the later broken branch, it no longer fires for the Thinkpad button.

@nh2

This comment has been minimized.

nh2 commented Jul 30, 2017

Looks like you are right: A quick bisect shows that the problem is introduced in this commit:

8e7dd00 Update to latest Glutin/winit (#671)

@nh2

This comment has been minimized.

nh2 commented Jul 30, 2017

I'm not sure how to continue debugging -- neither winit nor glutin seem to have have hangelog files that describe what changed between the releases.

@tomaka Do you have an idea what might have broken scroll support of the Thinkpad middle mouse button?

@tomaka

This comment has been minimized.

tomaka commented Jul 30, 2017

The glutin fork dated back from august 2016, so finding the commit that causes the problem might be hard. Winit and glutin shouldn't have had any change in their behaviour when it comes to scrolling, so it has to be a bug.

@nh2

This comment has been minimized.

nh2 commented Jul 30, 2017

@tomaka I'm new to glutin/winit and Rust in general, but I'd be happy to help with finding this problem.

I see there are some examples in glutin/winit. Could I use any of them to bisect this? E.g. some example that simply prints scroll events?

@nh2

This comment has been minimized.

nh2 commented Jul 30, 2017

@tomaka Well so the first thing I notice is that with glutin's target/debug/examples/window:

On glutin 0.6.1 (glutin commit e18ff88d) I get the output

MouseWheel(LineDelta(0, 1), Moved)
MouseWheel(LineDelta(0, 1), Moved)
MouseWheel(LineDelta(0, 1), Moved)
MouseWheel(LineDelta(0, 1), Moved)
MouseWheel(LineDelta(0, -1), Moved)
MouseWheel(LineDelta(0, -1), Moved)

while on current glutin master 9625c81c3 I get the output:

WindowEvent { window_id: WindowId(X(WindowId(81788931))), event: MouseWheel { device_id: DeviceId(X(DeviceId(2))), delta: LineDelta(0, -1), phase: Moved } }
DeviceEvent { device_id: DeviceId(X(DeviceId(13))), event: Button { button: ButtonId(5), state: Pressed } }
DeviceEvent { device_id: DeviceId(X(DeviceId(13))), event: Button { button: ButtonId(5), state: Released } }

If on current master I use a real mouse wheel I get

WindowEvent { window_id: WindowId(X(WindowId(81788931))), event: MouseWheel { device_id: DeviceId(X(DeviceId(2))), delta: LineDelta(0, 1), phase: Moved } }
DeviceEvent { device_id: DeviceId(X(DeviceId(11))), event: Motion { axis: AxisId(2), value: 1 } }
WindowEvent { window_id: WindowId(X(WindowId(81788931))), event: MouseWheel { device_id: DeviceId(X(DeviceId(2))), delta: LineDelta(0, -1), phase: Moved } }
DeviceEvent { device_id: DeviceId(X(DeviceId(11))), event: Motion { axis: AxisId(2), value: 1 } }

Not sure yet if that provides any insight?

@nh2

This comment has been minimized.

nh2 commented Jul 30, 2017

With some more bisecting I found that the change in winit from

WindowEvent { window_id: WindowId(X(WindowId(139764517513424))), event: MouseWheel(LineDelta(0, -1), Moved) }
WindowEvent { window_id: WindowId(X(WindowId(139764517513424))), event: MouseWheel(LineDelta(0, 1), Moved) }
WindowEvent { window_id: WindowId(X(WindowId(81788929))), event: MouseInput { device_id: DeviceId(X(DeviceId(2))), state: Released, button: Other(4) } }
DeviceEvent { device_id: DeviceId(X(DeviceId(13))), event: Button { button: ButtonId(4), state: Pressed } }
DeviceEvent { device_id: DeviceId(X(DeviceId(13))), event: Button { button: ButtonId(4), state: Released } }

is in commit 22bc119c... Richer input events

@nh2

This comment has been minimized.

nh2 commented Jul 30, 2017

CC @Ralith (who authored that change) -- do you know if the change may have broken ThinkPad middle mouse button scrolling?

@nh2

This comment has been minimized.

nh2 commented Jul 30, 2017

@tomaka @Ralith I suspect it might be this change that removed the translation of Button4/Button5 to MouseWheel events:

tomaka/winit@22bc119#diff-2e05a82bc4ad5f1e53602d126bbe42acL218-L227

nh2 added a commit to nh2/alacritty that referenced this pull request Jul 30, 2017

Fix scrolling with Middle mouse button + TrackPoint.
See jwilm#657 (comment)

The glutin change
  tomaka/winit@22bc119#diff-2e05a82bc4ad5f1e53602d126bbe42acL218-L227
removed the translation from mouse buttons 4 and 5 to MouseWheel events.

As a result, we now have to handle them ourselves in
Alacritty to make scrolling happen with those.
@nh2

This comment has been minimized.

nh2 commented Jul 30, 2017

@neon64 OK, here's a commit on top of your branch that fixes scrolling with middle mouse button + TrackPoint: nh2@be4c141

@tomaka @Ralith What we still need to know from you guys is whether the removal of translate_event() that translated Button4/Button5 events to MouseWheel events was intentional or not, to decide whether it should be handled in Alacritty (and all other apps that use glutin/winit) or reintroduced in winit.

Do you have opinions on this?

@tomaka

This comment has been minimized.

tomaka commented Jul 30, 2017

I guess it wasn't intentional. Since Button4 and 5 are mouse wheels, there's no point in reporting them as button presses anyway. But I'm waiting on Ralith's input.

@nh2

This comment has been minimized.

nh2 commented Jul 30, 2017

Another question @neon64:

Should Shift + PageUp/PageDown be correctly scrolling up and down currently?

For me it doesn't, only inserts 2~ into the prompt.

Some relevant section in alacrity.yml: - { key: PageUp, mods: Shift, chars: "\x1b[5;2~" }

Other terminals (e.g. gnome-terminal) handle that this way:

  • If they are in "normal" mode, then the mentioned key combinations scroll them.
  • If they run something that takes control of rendering, such as screen, then they forward the \x1b[5;2~.

Does Alacritty currently know whether an application like screen is running that uses that "takes control of the terminal" mode (I don't remember what that is called properly)?

@Ralith

This comment has been minimized.

Ralith commented Jul 30, 2017

CC @Ralith (who authored that change) -- do you know if the change may have broken ThinkPad middle mouse button scrolling?

It looks like this is the case. When I wrote the change, I determined empirically that both touchpad and real mousewheel scrolling yielded motion events in addition to emulated clicks, and concluded that said motion events could therefore be relied upon. Under libinput, this is true even for trackpoints. Unfortunately, it appears that the specific case of wheel emulation used for the trackpoint scrolling under evdev does indeed fail to produce motion events, so winit should support this.

Fix on the way at tomaka/winit#248.

@tbodt

This comment has been minimized.

Contributor

tbodt commented Jul 30, 2017

@nh2 If your alacritty.yml is configured to send ^[[5;2~ on shift+pageup, it's going to send ^[[5;2~ on shift+pageup, not scroll.

@neon64

This comment has been minimized.

neon64 commented Jul 31, 2017

@nh2 As part of my initial commit, I added ScrollUp and ScrollDown actions which you can bind to any key you want, and yes, it seems like this would be something good to have in the default config if it is expected behaviour from other terminals... Would you desire PageUp and PageDown actions as well? (because I imagine those could be easily implemented as needed)

I think you're referring to raw mode, although I'm not familiar enough with the rest of the codebase to know if/where Alacritty tracks that.

@nh2

This comment has been minimized.

nh2 commented Jul 31, 2017

If your alacritty.yml is configured to send ^[[5;2~ on shift+pageup, it's going to send ^[[5;2~ on shift+pageup, not scroll.

@tbodt Right, that makes sense to me, I'm just wondering if Alacritty should also make a distinction / allow to configure different actions between that raw mode and "normal" mode, like other terminals do.

Would you desire PageUp and PageDown actions as well?

@neon64 I think so -- at least I use them all the time in both UI terminals and the Linux virtual terminals.

These keybindings are common:

  • Ctrl+Shift+Up/Down - scrolls a single line, that's ScrollUp / ScrollDown that you've already implemented.
  • Shift+PgUp/PgDown - scrolls an entire page, that's the PageUp / PageDown that you suggest
  • Shift+Home/End - scrolls to the top/bottom
@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Dec 27, 2017

I've compared the current master to this PR with the new benchmarking tool @jwilm wrote, here are the results:

Alt Screen Random Write First Run Second Run Third Run
Master 0.950 0.933 0.918
PR 0.983 0.988 0.978

This test doesn't really do much related to scrolling as far as I know, so as expected, the results are pretty much even.

Scrolling First Run Second Run Third Run
Master 1.512 1.633 1.582
PR 1.070 1.166 1.110

This one has something to do with scrolling and as expected shows some more different results. I've checked this twice because I didn't expect the PR to be ahead here, but it definitely is ahead by more than just the margin of error and both runs have shown this.

Scrolling In Region First Run Second Run Third Run
Master 1.399 1.409 1.360
PR 2.108 2.070 2.105

This test showed the biggest difference between the two. I'm not quite sure why the gap is so massive here when the PR is ahead in the normal scrolling, but it falls behind significantly in this benchmark. I've also run this a couple of times and wasn't able to get under 2 seconds once with the PR.

This was all tested on my Arch/X/i3/ZSH machine.

I did not run the current master of vtebench but made a change so the scrolling benchmarks would run in the primary screen buffer, except for this everything is unchanged.

- { key: Home, chars: "\x1bOH", mode: AppCursor }
- { key: Home, chars: "\x1b[H", mode: ~AppCursor }
- { key: End, chars: "\x1bOF", mode: AppCursor }
- { key: End, chars: "\x1b[F", mode: ~AppCursor }
- { key: PageUp, mods: Shift, chars: "\x1b[5;2~" }
- { key: PageUp, mods: Control, chars: "\x1b[5;5~" }
#- { key: PageUp, mods: Control, chars: "\x1b[5;5~" }

This comment has been minimized.

@FichteFoll

FichteFoll Dec 28, 2017

I think you mean to comment out the Shift line, not the Control one.

@FichteFoll

This comment has been minimized.

FichteFoll commented Dec 28, 2017

I started using this as my main terminal now and so far it's been working nicely.

The only thing I noticed is that scrolling with the scroll wheel is not supported while in the alternate buffer (i.e. when viewing a man page).

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Dec 28, 2017

Scrolling in the alternate buffer is called faux scrolling and not part of this PR yet. On neon's github there is currently an open PR about rebasing on top of master, which will add the faux scrolling feature to this PR. But it still needs some tweaks before it can be merged.

@maximbaz

This comment has been minimized.

Contributor

maximbaz commented Jan 5, 2018

Just to understand, is there anything mandatory left to do here before this can be merged? It already works well and has been tried by many people (there is even a second AUR package created for alacritty, specifically based on this PR), and the change itself is huge making it difficult to review new additions.

I've noted ideas to potentially add more tests and optimize "Scrolling in region" (as reported by @chrisduerr), can we do them in separate PRs?

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Jan 5, 2018

@maximbaz I've talked to @jwilm in IRC and he told me that he wants to investigate a different approach to scrollback and see if that might be a more beneficial implementation.

So while this PR is in a good state right now, it will probably have to wait until @jwilm has some results on the different approach.

@ianks

This comment has been minimized.

ianks commented Jan 6, 2018

IMO, scrollback is a necessary feature that far outweighs prospective perf gains. Merge it in, then optimize if needed. This PR is now 6 months old. A lot of people (including myself) won't use alacritty without this feature.

@Kommynct

This comment has been minimized.

Kommynct commented Jan 7, 2018

I completely agree, it's already seemingly more performant than urxvt, I really want to replace termite with it as well.

@FichteFoll

This comment has been minimized.

FichteFoll commented Jan 7, 2018

The only issue I still have is that lines are not re-wrapped when the window shrinks and are truncated instead. If you enlarge the window again, the truncated characters do not come back either, presumably because they are pruned from memory entirely.

I don't know whether that is relevant to this PR in particular or "not a feature" of alacritty in general, but that's the only remaining thing from a usability standpoint.
(When working with a tiling WM, windows are resized frequently and losing the previously printed text is not so nice.)

@maximbaz

This comment has been minimized.

Contributor

maximbaz commented Jan 7, 2018

Reflow will be implemented separately, subscribe to #591. I think tmux could also handle reflows even if alacritty doesn't natively support this (I'm not sure thought), but current implementation of scrollback doesn't play well with tmux anyway - this is tracked by #1000.

@nixpulvis

This comment has been minimized.

Contributor

nixpulvis commented Jan 11, 2018

@maximbaz when you say separately do you mean using it's own buffer for the text? Or do you mean as a separate PR after this lands using the same facilities?

@FichteFoll

This comment has been minimized.

FichteFoll commented Jan 11, 2018

Painting is based on a grid structure, which most likely is a strict two-dimensional array (or array-like). For reflow, painting needs to rebuild the grid and wrap source lines into potentially multiple grid lines. That's hat I think at least.

In principle the implementation would be the same for current master and the scrollback PR, but the additional layer required to store the original line is kind of already there in the scrollback branch.

@FichteFoll

This comment has been minimized.

FichteFoll commented Jan 11, 2018

Regarding this alternative implementation, do we have any information about that? Like, what would it do different compared to this one and what's the current state of that?

@maximbaz

This comment has been minimized.

Contributor

maximbaz commented Jan 11, 2018

@maximbaz when you say separately do you mean using it's own buffer for the text? Or do you mean as a separate PR after this lands using the same facilities?

As a separate PR, not to delay this one any longer.

@alexozer

This comment has been minimized.

alexozer commented Jan 11, 2018

EDIT This comment can probably be ignored, I was setting an incorrect TERM in .zshrc to produce this.


I don't mean to unnecessarily delay this any longer (or whether I should file this separately), but it seems like the current scrollback implementation does not play nicely (plays incorrectly?) with programs using the alternative screen buffer.

For instance, opening and closing vim wipes some prior scrollback history:

# Generate a little output
echo '0\n1\n2\n3\n4\n5\n6\n7\n8\n9'
vim -c q # Open and close vim
# Observe some of echo's output missing

And fragments of a man page can be captured in the scrollback buffer:

man man
# [enter 'dq' into man]
# [scroll up to see part of man page]

I'm guessing this stems from the fact that the current scrollback implementation buffers the display instead of stdout and stderror. However, this random person on the internet seems to suggest that stdout and stderr in other terminals solely occupies the scrollback buffer and that other display-filling programs like vim and man don't ever touch the scrollback buffer since they use the "alternative screen buffer".

[The scrollback buffer] contains all the text that has been displayed on the screen, including both standard output and standard error from every program you run in the terminal.
...
Unlike the ordinary buffer that's used for most terminal interaction with sequential output, the alternate screen buffer is just the exact size of your terminal. There's no scrolling up or down in it because it's no bigger than what's displayed.
The idea there is to allow a full-screen application to do what it needs to do without being interfered with by anything you already had on the screen, and then to let you go back to exactly the display you had before.

So, I'm guessing that alternative implementation would avoid these issues?

@maximbaz

This comment has been minimized.

Contributor

maximbaz commented Jan 11, 2018

I was told on IRC that the alternative implementation will not cover this, so I made #1000 for this, but gave it a name specific to tmux only - I will rename now to better reflect the request.

@atopia

This comment has been minimized.

atopia commented Jan 11, 2018

@alexozer I can't reproduce this here (Arch package alacritty-scrollback-git-0.1.0.754.ga0b55ca-1 on xorg 1.19.6-2 with Awesome). Not sure if that could cause it, but do you have the appropriate terminfo files?

@alexozer

This comment has been minimized.

alexozer commented Jan 11, 2018

@atopia Ah I think you're right, I was setting TERM to something else manually in zshrc which now appears to produce the behavior I was seeing. I'll retract my comment.

@maximbaz you might want to update your issue because it might truly only affect tmux.

@Klemetz

This comment has been minimized.

Klemetz commented Jan 15, 2018

Have the testcases that needs to be written been specified? Seems hard to pitch in without any specifications.

@NellyWhadsDev

This comment has been minimized.

NellyWhadsDev commented Jan 30, 2018

Is there a way I can start using this feature before it hits master? Should I just check out this branch and compile + follow regular install procedure?

@FichteFoll

This comment has been minimized.

FichteFoll commented Jan 30, 2018

That's what I did, anyway. If you're on Arch, there is even a package for this branch on the AUR.

@theduke

This comment has been minimized.

theduke commented Mar 2, 2018

@jwilm Considering the PR is approaching it's first birthday, I think some statement from you is needed here.

What's the likely future of this branch?
Are you exploring an alternative approach, as was mentioned above?
Is this just blocked on rebasing, thorough testing and bug fixing?
Will this ever be merged?

@chetgurevitch

This comment has been minimized.

Contributor

chetgurevitch commented Mar 2, 2018

Just for the record I've got an open pull request to @neon64 rebasing this to master neon64#10.

@jwilm jwilm referenced this pull request Mar 9, 2018

Merged

Scrollback #1147

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Mar 9, 2018

See #1147 for the alternative scrollback implementation.

@valignatev

This comment has been minimized.

valignatev commented Mar 19, 2018

@chetgurevitch Compiled your rebased PR on Arch without any problem and using it for about 2 weeks without any issue. Much love ❤️

@jwilm jwilm closed this in #1147 Sep 17, 2018

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