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

Add scrollbar #7231

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

flash-freezing-lava
Copy link

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

@kchibisov
Copy link
Member

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 kchibisov requested review from chrisduerr and kchibisov and removed request for chrisduerr September 20, 2023 18:31
@flash-freezing-lava
Copy link
Author

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.

Copy link
Member

@chrisduerr chrisduerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some quick high-level things I've noticed:

  • Way too many config options
  • Mouse interactivity is fine as long as it is simple
  • Code needs cleanup
  • 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)
  • Don't use let else
  • No framerate limit on fading?
  • 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

Comment on lines 1749 to 1753
let grid_width = cell_width * dimensions.columns.0.max(MIN_COLUMNS) as f32;
let grid_height = cell_height * dimensions.lines.max(MIN_SCREEN_LINES) as f32;

let width = (padding.0).mul_add(2., grid_width).floor();
let width = (padding.0).mul_add(2., grid_width).floor()
+ config.scrollbar.additional_padding(scale_factor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this takes the existing UI lines into account?

Copy link
Author

@flash-freezing-lava flash-freezing-lava Sep 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify what you mean? I think the window_size is always independent of what UI lines we currently have. The scrollbar padding is just added in the same way as other padding, and only depends on config and window scale_factor.

@nixpulvis
Copy link
Contributor

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
Copy link
Contributor

nixpulvis commented Sep 27, 2023

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
Copy link
Contributor

nixpulvis commented Sep 27, 2023

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.

@flash-freezing-lava
Copy link
Author

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.

@kchibisov
Copy link
Member

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.

@nixpulvis
Copy link
Contributor

@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.

@flash-freezing-lava flash-freezing-lava deleted the non-interactive-scrollbar branch September 30, 2023 15:57
@flash-freezing-lava flash-freezing-lava restored the non-interactive-scrollbar branch September 30, 2023 15:59
@flash-freezing-lava flash-freezing-lava force-pushed the non-interactive-scrollbar branch 2 times, most recently from 4966639 to 27dd4e3 Compare September 30, 2023 16:09
@flash-freezing-lava flash-freezing-lava changed the title Add non-interactive scrollbar Add scrollbar Sep 30, 2023
@flash-freezing-lava
Copy link
Author

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.

@kchibisov
Copy link
Member

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

@flash-freezing-lava
Copy link
Author

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

@nixpulvis
Copy link
Contributor

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
Copy link
Contributor

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.


Default: _0.5_

*fade_time_in_secs* <float>
Copy link
Contributor

@nixpulvis nixpulvis Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be more consistent with other options like the visual bell, could we name this duration and make it an integer in milliseconds? This is kinda verbose.

@nixpulvis
Copy link
Contributor

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
Copy link
Contributor

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.

@mymtw
Copy link

mymtw commented Mar 21, 2024

will we get this feature?

@nixpulvis
Copy link
Contributor

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

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

Successfully merging this pull request may close these issues.

5 participants