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

?

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.

In D13582#279300, @hindenburg wrote:

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.

In D13582#279299, @tcanabrava 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. ;-)

In D13582#279304, @zzag wrote:

In D13582#279299, @tcanabrava 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?

In D13582#279305, @tcanabrava wrote:

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?

In D13582#279370, @ngraham wrote:

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

In D13582#279371, @tcanabrava wrote:

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?

In D13582#279540, @ngraham wrote:

In D13582#279371, @tcanabrava wrote:

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?

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.

In D13582#282423, @Fuchs wrote:

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?

In D13582#283071, @tcanabrava wrote:

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

See T9126: Breeze scrollbar look-and-feel unification