ScrollBar overlay theme aware.
ClosedPublic

Authored by tcanabrava on Jun 17 2018, 3:40 PM.

Details

Reviewers
hindenburg
ngraham
Group Reviewers
Konsole
Summary

Make the scrollbar theme aware.
To simplify code I'v extracted the ScrollBar to it's own class, it's not much as it has around 100 LOC but I felt it was not something that the TerminalDisplay should have to deal with.
Now the scrollbar will behave as overlay if theme is breeze, and no if it's anything else.

Test Plan

Opened konsole and changed themes.

Diff Detail

Repository
R319 Konsole
Branch
fixThemeScroolBarOverlay
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 79
Build 79: arc lint + arc unit
tcanabrava created this revision.Jun 17 2018, 3:40 PM
Restricted Application added a project: Konsole. · View Herald TranscriptJun 17 2018, 3:40 PM
Restricted Application added a subscriber: konsole-devel. · View Herald Transcript
tcanabrava requested review of this revision.Jun 17 2018, 3:40 PM
tcanabrava retitled this revision from Extract ScrollBar to it's own Class to ScrollBar overlay theme aware..Jun 17 2018, 3:44 PM
tcanabrava edited the summary of this revision. (Show Details)
tcanabrava edited the test plan for this revision. (Show Details)
tcanabrava added reviewers: Konsole, hindenburg, ngraham.
zzag added a subscriber: zzag.EditedJun 17 2018, 4:11 PM

Now the scrollbar will behave as overlay if theme is breeze, and no if it's anything else.

So, as far as I understand, there would be an overlay scrollbar only if using Breeze widget style. What about other widget styles? Maybe add some option to configure scrollbar mode, e.g.

Scrollbar mode: [ ] Static
                [x] Overlay

?

tcanabrava added a comment.EditedJun 17 2018, 4:18 PM
In D13582#279296, @zzag wrote:

Now the scrollbar will behave as overlay if theme is breeze, and no if it's anything else.

So, as far as I understand, there would be an overlay scrollbar only if using Breeze widget style. What about other widget styles? Maybe add some option to configure scrollbar mode, e.g.

Scrollbar mode: [ ] Static
                [x] Overlay

?

User chooses Oxygen, User chooses Scrollbar mode: Overlay
Broken Terminal as he can't read the last few characters of his text.

The problem is not a lack of options, but we can only enable overlay for styles that are translucent on the toolbar, and currently for all styles that ships with Plasma, Breeze is the only one that works

I'm unable to get the non-overlay to show up after changing to use non breeze themes like Oxygen, Air.

I'm unable to get the non-overlay to show up after changing to use non breeze themes like Oxygen, Air.

Hm, I think currently a resize is needed.
I requested a parentWidget()->update(); but I think that's not enougth.

zzag added a comment.Jun 17 2018, 4:24 PM

User chooses Oxygen, User chooses Scrollbar mode: Overlay
Broken Terminal as he can't read the last few characters of his text.

Yes, you're totally right it would be broken with Oxygen widget style. But there could be other widget styles with transparent scrollbars. Anyway, that was just a suggestion, feel free to ignore that.

The problem is not a lack of options, but we can only enable overlay for styles that are translucent on the toolbar, and currently for all styles that ships with Plasma, Breeze is the only one that works

A user can install other widget styles from store dot kde dot org. ;-)

In D13582#279304, @zzag wrote:

User chooses Oxygen, User chooses Scrollbar mode: Overlay
Broken Terminal as he can't read the last few characters of his text.

Yes, you're totally right it would be broken with Oxygen widget style. But there could be other widget styles with transparent scrollbars. Anyway, that was just a suggestion, feel free to ignore that.

The problem is not a lack of options, but we can only enable overlay for styles that are translucent on the toolbar, and currently for all styles that ships with Plasma, Breeze is the only one that works

A user can install other widget styles from store dot kde dot org. ;-)

I'll never ignore input and I don't want to hold users back but I think that enabling a feature that will not work with every theme is dangerous unless we blacklist a few.
Would it work for you a option "Overlay Scrollbar for this theme" plus a warning that it would only look good if the theme supports translucency and it doesn't have the up / down arrows?

zzag added a comment.Jun 17 2018, 8:22 PM

I'll never ignore input and I don't want to hold users back but I think that enabling a feature that will not work with every theme is dangerous unless we blacklist a few.

Again, that was just a suggestion.

Would it work for you a option "Overlay Scrollbar for this theme" plus a warning that it would only look good if the theme supports translucency and it doesn't have the up / down arrows?

Well, not sure. I think it would be better to ask @ngraham and @hindenburg what they think about that.

It's beginning to look like the "Use overlay scrollbars?" setting might be better located in the Breeze theme settings. That way it would only affect Breeze, and we'd also have a single centralized setting that apps could check to determine whether to draw overlay scrollbars or not. Also, the setting would be in one place, and once we make KDE apps overlay-scrollbar-setting-aware, changing it there would affect ll KDE apps, not just Konsole.

Thoughts?

It's beginning to look like the "Use overlay scrollbars?" setting might be better located in the Breeze theme settings. That way it would only affect Breeze, and we'd also have a single centralized setting that apps could check to determine whether to draw overlay scrollbars or not. Also, the setting would be in one place, and once we make KDE apps overlay-scrollbar-setting-aware, changing it there would affect ll KDE apps, not just Konsole.

Thoughts?

I don't know how that will work out Layout wise. When we have the scrollbar as overlay we ignore the Layout (konsole manually set's the position of the scrollbar above the drawarea of the widget for instance)
I have no clue on how that's supposed to work

I don't know how that will work out Layout wise. When we have the scrollbar as overlay we ignore the Layout (konsole manually set's the position of the scrollbar above the drawarea of the widget for instance)
I have no clue on how that's supposed to work

Without being so familiar with how it would work technically, it seems like if we did this, then for Breeze with the setting checked, it would just draw the scrollbar overlay-style, whereas for other themes or for Breeze with the setting unchecked, it would draw the scrollbar "normally".

Is that not possible?

I don't know how that will work out Layout wise. When we have the scrollbar as overlay we ignore the Layout (konsole manually set's the position of the scrollbar above the drawarea of the widget for instance)
I have no clue on how that's supposed to work

Without being so familiar with how it would work technically, it seems like if we did this, then for Breeze with the setting checked, it would just draw the scrollbar overlay-style, whereas for other themes or for Breeze with the setting unchecked, it would draw the scrollbar "normally".

Is that not possible?

I don't know how technically that would work. the QLayout asks for the sizeHint() of the widget to adapt it's space (so for breeze we would need to return 0 as sizeHint() ) - but I *think* this is also what's used for the paintEvent to paint it. I'm currently hacking breeze style to see if I can do that.

I'd like to have new features for 18.08 in master within 2 weeks. That will give about 2 weeks of people using distributions using master (neon/gentoo/arch) to test it before the rc#s.

Do we need to revert the entire scrollbar overlay until after 18.08 is branched?

ngraham accepted this revision.Sun, Jun 24, 1:11 PM

In VDG land, this has led to an interesting conversation regarding how we want to handle overlay scrollbars more generally. However that's going to be more long-term work, so I think this patch is fine to go in as-is so that we can support the on-demand scrollbar feature for 18.08.

This revision is now accepted and ready to land.Sun, Jun 24, 1:11 PM
Fuchs added a subscriber: Fuchs.Sun, Jun 24, 2:18 PM

Personally I don't think that we should ship changes that rely on hard-coded theme names like

_canFloat = qApp->style()->objectName() == QLatin1Literal("breeze");

as it will already fail to work for variants of that theme.

I also think that if we already know that this needs to be tackled and will be tackled more globally, in a place that makes more sense and is way less of a workaround, we should maybe wait and skip a version, to ship something that works well and is in a state we are happy with and not a state we will already change again in the forseeable future.

Short: I'd vote for having it indeed as a setting in breeze instead of a workaround in konsole.

Personally I don't think that we should ship changes that rely on hard-coded theme names like

_canFloat = qApp->style()->objectName() == QLatin1Literal("breeze");

as it will already fail to work for variants of that theme.

I also think that if we already know that this needs to be tackled and will be tackled more globally, in a place that makes more sense and is way less of a workaround, we should maybe wait and skip a version, to ship something that works well and is in a state we are happy with and not a state we will already change again in the forseeable future.

Short: I'd vote for having it indeed as a setting in breeze instead of a workaround in konsole.

Fair Enough.
if this doesn't go thru konsole we need to remove a few commits that are related to scrollbar overlay and scrollbar auto hide.
I'v looked at breeze and I don't know how to tackle there as the scrollbars are usually added as a layout item and that item will need to have it's size set to 0 so the item on the right / left can go underneath it - non trivial.
I hope to be wrong here. :)

I'll wait for nathan's input and if necessary close this review and revert some commits.

Ultimately I think it should be @hindenburg's decision. I agree with all of @Fuchs points and can't really object to any of them. Correctness is important after all.

Let's revert the scrollbar overlay for 18.08; we can put it back in once 18.08 is branched off of master.

Kurt, I'm holding this as requested, but we have *some* code already for that in konsole, can I just git revert it?

Kurt, I'm holding this as requested, but we have *some* code already for that in konsole, can I just git revert it?

yes git revert is often the easiest

tcanabrava closed this revision.Wed, Jul 4, 6:43 AM

Decided to implement in breeze as a whole.