alacritty icon indicating copy to clipboard operation
alacritty copied to clipboard

Add scrollbar

Open flash-freezing-lava opened this issue 2 years ago • 17 comments

To address the need for a visual indicator, I implemented a non-interactive scrollbar with 3 modes, selectable via config:

  1. Never (default): This disables the scrollbar.
  2. Fading: Shows the scrollbar after the view changed (e.g. by scrolling, adding new text, or resizing the window) and fades it out after a short time. Does not reserve space for the scrollbar, but draws it semi-transparent over the view area.
  3. Always: Reserves space for the scrollbar. The scrollbar is permanently shown, unless scrolling is impossible, because there is no content out of view.

The mode and further configurations are documented in the manpage.

Of the different features discussed in #775, this PR only addresses the need for a visual indicator, not for new ways to navigate the scrollback. If there is interest to merge such a change, I would be willing to implement dragging functionality to the scrollbar.

Fading mode

https://i.imgur.com/FYyMJtc.mp4

Always mode

https://i.imgur.com/BcUkKq9.mp4

flash-freezing-lava avatar Sep 20 '23 17:09 flash-freezing-lava

I haven't looked very deep into the implementation nor that I tested, but I have a few questions based on the quick glance:

  1. Have you considered making it similar to the way search bar works in terms of sizing? For example maybe it can simply be configured in terms of the width of the cell and say, that minimum height will be clamped by the height of the cell?
  2. How this will interact with the Vi mode, which also has an indicator, but a bit different?
  3. Have you thought on defining a fixed animation or reusing the ones we have for the visual bell (maybe only subset of them)? I think that logic could be reused a lot, since visual bell and scrollbar are animating the same basic object (rectangle with alpha). I don't think that you can configure scrollbar animations in software in general, and less config options is better.
  4. One concern is how selection should work with the Fading option you showed, since if this scrollbar can be dragged with the mouse, it'll conflict with the selection and Vi mode cursor. I tend to think that Always showed option could be better? Or maybe fading could be done for the Always cursor and you'll just have a constant offset on the right and you'll get a scrollbar visible once you move your mouse or something?

Without mouse drag I'm pretty sure it's not that helpful, since mouse users want to fast scroll the terminal sometimes. Though, I don't think that it'll be that complicated to add, given that with Always mode it's relatively simple.

I'm in general +1 on adding this feature, because it has practical use and given that it's relatively simple, since you just offset and draw rectangle, which is basically what search does already, but you offset from the right side instead, and do similar offsetting in resize methods.

I'd wait for @chrisduerr to comment on that matter, but I know that the amount of config options should be cut, and that without a mouse/touch drag, it'll be hard to sell this feature. On mobile devices scrollbar will help a lot, since you can select and scroll with the scrollbar at the same time basically, and scrolling without kinetic scroll, etc is painfully slow there.

kchibisov avatar Sep 20 '23 18:09 kchibisov

To answer some of the questions:

  1. I did not consider fixing the size and margins by cell sizes, but that would be a way to reduce config options while still respecting the user's preferences for sizing.

  2. In VI mode, the indicator is drawn over the cell area and is therefore handled the same way as the console text. In Fading mode, the scrollbar is drawn over it. In Always mode, where the scrollbar reserves space, the scrollbar appear to the right of the indicator.

  3. Since I never used the visual bell, I wasn't aware it does has similar logic. I will look into whether the animation logic can be shared or reused. Though I wouldn't want to use the bell's duration config for scrollbar fading, as that would require users of a fading scrollbar, to also use the visual bell.

  4. For the Always mode, there would be no conflict between scrollbar and selection. For Fading mode, I planned to have a click on the scrollbar trigger scrollbar dragging and no selection. To start a selection at a cell, that is shadowed by the scrollbar, you would have to wait until the scrollbar is faded out.

I would wait for directions before with implementing mouse dragging, because one (old) comment from @chrisduerr under #775 was: A scrollback that can be dragged around is definitely out of scope of this project. The main reason why this issue is still open, because an indicatior to show position in the scrollback buffer is not completely out of question. . I agree, that support for mouse dragging shouldn't be too complicated.

I personally think a non-interactive scrollbar is already a usage improvement compared to no scrollbar, as it provides more intuitive orientation than the line numbers in the VI indicator, though I agree, that a draggable scrollbar would be a much bigger improvement for mouse users.

flash-freezing-lava avatar Sep 20 '23 21:09 flash-freezing-lava

This is super cool!

Have you considered making it similar to the way search bar works in terms of sizing? For example maybe it can simply be configured in terms of the width of the cell and say, that minimum height will be clamped by the height of the cell?

This was one of the first things I noticed. As it stands the scrollbar itself gets so small that it is somewhat lost in the window rounding on macOS. I wouldn't even be surprised if the scrollbar minimum height was two cells tall TBH.

How this will interact with the Vi mode, which also has an indicator, but a bit different?

I'm not sure I mind having the two indications on screen at once, since one is the well known scrollbar, while the other tells you exactly what line the cursor is on. However, I'd be curious if these features could be unified somewhat?

On a very small note, and perhaps this is just my bias, but I think the default fadeout durations should be a bit faster. Both the delay before fade and the fade themselves.

nixpulvis avatar Sep 27 '23 17:09 nixpulvis

Digging a bit deeper into the current config options here's some more thoughts:

Setting the min_height to 25 seems decent to me, but I still like the idea of removing this config option altogether and just using a sane default. I'm starting to thinks that it shouldn't be related to the cell height though. Think about it, on a web browser, you can scale the content, but it doesn't change the scrollbar size itself.

Additionally, there's a small bug here I think. The actual position of the scrollbar appears to be the middle of the bar, however, it should be the bottom of the scrollbar when at the bottom, and the top of the scrollbar when at the top. This way the scrollbar is always completely inside the display. Just a little bit more math required to offset the bar needed.

Is there any reason to not just disable the scrollbar altogether when the opacity is 0.0? Should this even be configurable? I get that color probably needs to be configurable, unless there's a smart way to choose an inverse of the background color, but 0.5 seems like a totally sane value for the fading mode here to me.

nixpulvis avatar Sep 27 '23 18:09 nixpulvis

Finally, I'll add that the current interaction between window.padding and scrollbar.margin seems a bit funky to me. I feel like it's sort of weird to have the scrollbar essentially outside of the window, if you think of this like the box model. But frankly, I don't see a good reason for this to be configurable. Once the scrollbar is tall enough and clamped to the screen (see my last comment), then the margin.y becomes less critical and potentially confusing, since it may lead you to think there's more to scroll than there is. margin.x being set to 0 seems like a perfectly fine setting to me.

nixpulvis avatar Sep 27 '23 18:09 nixpulvis

Additionally, there's a small bug here I think. The actual position of the scrollbar appears to be the middle of the bar, however, it should be the bottom of the scrollbar when at the bottom, and the top of the scrollbar when at the top. This way the scrollbar is always completely inside the display. Just a little bit more math required to offset the bar needed.

Thanks for testing. It should be fixed now on the wip-branch.

flash-freezing-lava avatar Sep 27 '23 19:09 flash-freezing-lava

The height should likely be based on the amount of lines in scrollback and just have a minimum hieght of a cell height/or two.

kchibisov avatar Sep 27 '23 23:09 kchibisov

@kchibisov I was just referring to the min height.

If anything though, it should be based on the window height, not the cell height. I think I'd find it strange for the height of the scrollbar to change for any reason other than the amount of content in the scrollback changing. It's also simpler to make it a constant amount.

nixpulvis avatar Sep 27 '23 23:09 nixpulvis

I have updated several aspects:

Reduced config options

The x-margin was replaced by reusing the window padding as margin between text and scrollbar. The y-margin was removed. The scrollbar width and minimum height is now one cell wide and two cells high. Having the height based on cell size can be unintuitive, as the scrollbar gets larger if you increase text size. I tried to use a value based on window height instead, but couldn't find a reasonable value for both large and small screens. Basing height on cell size should be very safe in that regard, as every window, whether on a large screen or a mobile device, must have a well visible and not too large cell size.

Mouse interactibity

The scrollbar is now draggable by mouse and touch events.

Others

Scrollbar doesn't correctly indicate no scrollback, it is hidden rather than being the full height of the terminal (this matches browsers, but I'm not sure it makes sense for "always", vte always shows the scrollbar)

Scrollbar is no longer hidden if it would take up the whole screen height in Always mode, so that mode gets closer to other terminal emulators, while Fading mode stays closer to browser behavior.

Don't use let else

Let-else is now written out, as it like before rust 1.65.

No framerate limit on fading?

While waiting for the actual fading, the scrollbar is no longer redrawn. Instead a redraw is scheduled with a timer. For the animation itself (by default for 0.4 seconds), there is no throttling. The animation directly requests a redraw similar to the visual bell.

Display offset should not be stored in two locations, in fact it should be able to compute all necessary data from just the render state, rather than having extra scroll state

To know, whether the scrollbar must be redrawn (reset fading and add damage rect), there needs to be some tracking whether the view area changed since last redraw. As far as I know this isn't done yet, so I opted to have the scrollbar keep track of the changes relevant to it. Instead of storing display_offset and total_lines, the time of last view change could also be stored in Term and be updated on Term::scroll_display, Term::resize, and others. But I consider that a more error-prone implementation, as there are multiple actions (scroll, add new line, resize window) requiring a scrollbar redraw, and all would need to be aware of the this tracking. With the current implementation, the scrollbar knows itself, whether the state changed in a way that requires redrawing, which reduces the need for other existing or future parts of alacritty to be aware of the scrollbar.

flash-freezing-lava avatar Oct 07 '23 19:10 flash-freezing-lava

@flash-freezing-lava are you waiting on re-review? Though, I don't remember if you can re-request it.

kchibisov avatar Oct 17 '23 14:10 kchibisov

@kchibisov The request for your review should still be open.

flash-freezing-lava avatar Oct 19 '23 12:10 flash-freezing-lava

Small thing I noticed while giving this another look today.

Live reloading the config doesn't seem to work when the scrollbar was initially not present.

nixpulvis avatar Oct 30 '23 14:10 nixpulvis

Also, if we're modeling the behavior of the cursor after macOS at all (which isn't always the worst thing to do), we shouldn't change it to CursorIcon::RowResize since we're not really resizing anything.

I was somewhat surprised that the cursor didn't change to a Pointer on hover, but thinking more about it, it makes sense. I can't test how other scrollbars work on Windows and Linux systems right now, but I suspect they are the same as macOS in this respect.

More tiny UI/UX things.

nixpulvis avatar Oct 30 '23 14:10 nixpulvis

Not sure if this is the same issue as I'm having with the visual bell on macOS, but the scrollbar doesn't seem to go away in the fading mode until another action triggers a redraw.

Perhaps we can kill two birds with one stone in this fix?

nixpulvis avatar Oct 30 '23 15:10 nixpulvis

Last thing for now.

How would we feel about always drawing the top and bottom of the scrollbar on the absolute top and bottom of the window instead of inside a row of the grid? This ways A) it's more clear when you are at the top or bottom and B) the scrollbar doesn't jump around as much when vertically resizing.

This is generally more consistent with how other applications layout the scrollbar and it's controlling content. The scrollbar isn't inside of the content it scrolls.

nixpulvis avatar Oct 30 '23 15:10 nixpulvis

will we get this feature?

mymtw avatar Mar 21 '24 03:03 mymtw

I’m watching this, happy to test as needed.

nixpulvis avatar Mar 25 '24 19:03 nixpulvis