Show the scrollbar only when needed
ClosedPublic

Authored by anemeth on Mar 31 2018, 7:19 PM.

Details

Summary

It hides the scrollbar when there isn't enough lines to scroll.
This also affects KParts like Yakuake or Dolphin and others.

Test Plan

Diff Detail

Repository
R319 Konsole
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
anemeth created this revision.Mar 31 2018, 7:19 PM
Restricted Application added a project: Konsole. · View Herald TranscriptMar 31 2018, 7:19 PM
Restricted Application added a subscriber: Konsole. · View Herald Transcript
anemeth requested review of this revision.Mar 31 2018, 7:19 PM
anemeth edited the summary of this revision. (Show Details)Mar 31 2018, 7:24 PM
anemeth edited the test plan for this revision. (Show Details)
anemeth added reviewers: hindenburg, Konsole, VDG.
anemeth added a subscriber: ngraham.
anemeth updated this revision to Diff 31061.Mar 31 2018, 7:36 PM

Fixed accidentally inverted logic when changing scrollbar visibility settings

ngraham accepted this revision as: VDG.Mar 31 2018, 8:31 PM

I love it! Most other apps use this disappearing scrollbar style, and I think it makes sense for Konsole to follow suit. Very attractive. And since it's just showing and hiding--not actually changing the content area--no layout changes are necessary, which is nice.

Pitel added a subscriber: Pitel.Mar 31 2018, 8:36 PM

How does it work if text fills the whole line (or if you run mc or some other ncurses application)? Does it wrap the text once scrollbar is shown, hide it or is the text always wrapped in front of scrollbar's rect even if the scrollbar is not visible?

How does it work if text fills the whole line (or if you run mc or some other ncurses application)? Does it wrap the text once scrollbar is shown, hide it or is the text always wrapped in front of scrollbar's rect even if the scrollbar is not visible?

Pitel added a comment.Mar 31 2018, 8:45 PM

Thanks for the screenshot. I think this will raise questions "why is there the ugly black stripe on right"... (But I generally dislike all overlay and overaly-like-looking scrollbars on desktop, so I am biased.)

ngraham added a comment.EditedMar 31 2018, 8:48 PM

Without the patch, there's still an ugly back stripe on the side, it just has a useless scrollbar in it:

"why is there the ugly black stripe on right"

otherwise there would be a srollbar that you can't interact with
disabling the scrollbar area for these kind of terminal programs would be a different challenge not in scope for this patch

Pitel added a comment.Mar 31 2018, 9:03 PM

I know the stripe is still useless but user sees "it is disabled scrollbar, ok", not "empty black stripe, what is going on".

But I have to admit that with this scrollbar design, disabled one looks a bit weird. Is it new Breeze style, custom Konsole one or just usual one looking so different because terminal colors are applied to it?

zzag added a subscriber: zzag.EditedMar 31 2018, 9:49 PM

Without the patch, there's still an ugly back stripe on the side, it just has a useless scrollbar in it:

It would be great to have overlay scrollbars(optional).

Do KDE folks have objections against them?

mglb added a subscriber: mglb.Apr 1 2018, 1:07 AM

"why is there the ugly black stripe on right"

otherwise there would be a srollbar that you can't interact with
disabling the scrollbar area for these kind of terminal programs would be a different challenge not in scope for this patch

As I know, the second/alternative screen (the one where ncurses programs run) does not use history and scrolling by design, so a scrollbar area can always be used by a program.
If you want to play with it, take a look at TerminalDisplay::usingPrimaryScreen().

I'm not against this; however, when the scrollbar is not visible, the "extra" space where it should be is noticeable.

zzag added a comment.EditedApr 5 2018, 4:12 PM

Ping.

Is there any progress on solving the problem with extra space when scrollbar is hidden?

In D11843#240634, @zzag wrote:

Ping.

Is there any progress on solving the problem with extra space when scrollbar is hidden?

That is not in the scope of this patch.
I didn't work on it, as the patch is considered done from my side.
I am only waiting for @hindenburg to review the code.

Since the space that the scrollbar uses will always be there, is there any real reason to "hide" when not needed? I don't really see this as an improvement.

zzag added a comment.Apr 5 2018, 7:44 PM

That's really awkward when there is empty space on the right hand side

-1, sorry, I suggest to leave current scrollbars as is.

Most apps these days only show a scrollbar when necessary. However, they also don't have the limitation of this patch, and there's no strangely un-used area where a scroll bar would be.

I think for consistency's sake this is a valuable feature in general, but only if we can make it behave like other apps, and not just look like them. That probably means a more involved patch to automatically re-lay-out the lines when the scrollbar appears, which may make it look more like an implementation of the feature requested in https://bugs.kde.org/show_bug.cgi?id=196998--which would be amazing (613 votes!!!), but I imagine much more difficult.

In theory it sounds good to not just hide the scrollbar area, but disable it when not needed.
But in reality it would cause more issues.
Imagine this: The line is filled with "lu lu lu" or with a progress bar (for example apt-get progress bar) but when the user adds enough lines to add a scrollbar, all that text just jumps around to get around the layout and this would be more much distracting than a seemingly empty narrow area at the end of the line. To counter this Konsole would have to resize itself when that happens, but I believe is not possible in Wayland and would not be practical either.
That narrow empty scrollbar area is not as distracting as you think, maybe the first time the user encounters it, but then quickly discovers that a scrollbar just shows up there when necessary.
Only maybe a in an ncurses app, but to counter it we could show the (useless) scrollbar there. I don't know if that's a better solution or not, I leave that to the VDG to answer.

So do we agree that in Konsole's current state, this should not be applied?

FWIW there's an open patch to use overlay-style scrollbars, which should make this possible: D13543: Treat scrollbar as overlay

Restricted Application added a subscriber: konsole-devel. · View Herald TranscriptJun 14 2018, 4:46 PM

Can you rebase this please?

anemeth updated this revision to Diff 36211.Jun 15 2018, 9:12 PM

Rebase on master

hindenburg accepted this revision.Jun 16 2018, 6:09 PM

Thanks

This revision is now accepted and ready to land.Jun 16 2018, 6:09 PM
This revision was automatically updated to reflect the committed changes.