-
Notifications
You must be signed in to change notification settings - Fork 3k
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
base: master
Are you sure you want to change the base?
Add scrollbar #7231
Conversation
I haven't looked very deep into the implementation nor that I tested, but I have a few questions based on the quick glance:
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 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. |
To answer some of the questions:
I would wait for directions before with implementing mouse dragging, because one (old) comment from @chrisduerr under #775 was: I personally think a non-interactive scrollbar is already a usage improvement compared to no scrollbar, |
There was a problem hiding this 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
alacritty/src/display/mod.rs
Outdated
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Now is more similar to other terminals and less to browsers.
This is super cool!
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.
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. |
Digging a bit deeper into the current config options here's some more thoughts: Setting the 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 |
Finally, I'll add that the current interaction between |
Thanks for testing. It should be fixed now on the |
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 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. |
4966639
to
27dd4e3
Compare
08cf060
to
654cbb4
Compare
This fixes issues in tests, where alacritty is tested without display and therefore also without scrollbar. Previously it panicked because the display was unimplemented.
654cbb4
to
4fec552
Compare
I have updated several aspects: Reduced config optionsThe 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 interactibityThe scrollbar is now draggable by mouse and touch events. Others
Scrollbar is no longer hidden if it would take up the whole screen height in
Let-else is now written out, as it like before rust 1.65.
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.
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 |
@flash-freezing-lava are you waiting on re-review? Though, I don't remember if you can re-request it. |
@kchibisov The request for your review should still be open. |
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. |
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 I was somewhat surprised that the cursor didn't change to a More tiny UI/UX things. |
|
||
Default: _0.5_ | ||
|
||
*fade_time_in_secs* <float> |
There was a problem hiding this comment.
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.
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? |
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. |
will we get this feature? |
I’m watching this, happy to test as needed. |
To address the need for a visual indicator, I implemented a non-interactive scrollbar with 3 modes, selectable via config:
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