[Applet Configuration] Don't draw a separator between the titlebar and window
AcceptedPublic

Authored by filipf on Dec 4 2019, 2:28 AM.

Details

Reviewers
ngraham
Group Reviewers
Plasma
VDG
Summary

I added a top separator for QML applet configuration windows in D20908

This separator is the equivalent of KWin's separator, minus the highlight color.

I have been regretting the decision to draw it ever since the patch was committed and believe it to be wrong for several reasons:

  • it's an infrigment on user choice: it doesn't respect the fact the user has turned off KWin's titlebar separator
  • it's inconsistent with qwidgets: there is no top separator there
  • it's degrades the goal of trying to have a uniform appearance between the window manager and window in the case of Breeze Light and Breeze Dark because it distinctly points out the titlebar
  • it's superfluous (not visible) when used with the default color scheme anyway

This patch therefore removes the separator. For consistency reasons the separator will be drawn by the tools area code provided the option is turned on.

Test Plan

Before:

After:

Diff Detail

Repository
R119 Plasma Desktop
Branch
no-unwanted-titlebar-separator (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25478
Build 25496: arc lint + arc unit
filipf created this revision.Dec 4 2019, 2:28 AM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 4 2019, 2:28 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
filipf requested review of this revision.Dec 4 2019, 2:28 AM
filipf edited the test plan for this revision. (Show Details)Dec 4 2019, 2:29 AM
filipf added reviewers: Plasma, VDG.
ngraham added a subscriber: ngraham.Dec 4 2019, 5:45 AM

But... the "Before" image looks better. :/

Having the titlebar blend with the window kind of requires that nothing wants to be touching the titlebar, or else it just looks bad IMO. Personally my preference is to restore consistency by moving in the opposite direction by always showing a separator between what I'm calling the window's "tools area" (which consists of its titlebar + menubar if visible + toolbar if visible). That's what T10201 is all about and I had thought we had some agreement on that. If we land this patch, then there's no separator below the Tools Area for these windows, going against the goal of T10201.

broulik added a subscriber: broulik.Dec 4 2019, 7:35 AM

I never noticed that line before but now that I did, I can't unsee it. It looks quite bad with dark Breeze title bar, where it is quite superfluous.

But... the "Before" image looks better. :/

Having the titlebar blend with the window kind of requires that nothing wants to be touching the titlebar, or else it just looks bad IMO. Personally my preference is to restore consistency by moving in the opposite direction by always showing a separator between what I'm calling the window's "tools area" (which consists of its titlebar + menubar if visible + toolbar if visible). That's what T10201 is all about and I had thought we had some agreement on that. If we land this patch, then there's no separator below the Tools Area for these windows, going against the goal of T10201.

From a visual POV I understand, we want to "finish the grid". But consistency and philosophically-wise we find ourselves in an awkward place now.

We tell the people who don't want titlebars separators here that they should live with it, but to those who want titlebar separators in QWidgets config windows we tell that they should turn on KWin's option.

Why I actually don't see this patch going against T10201 is that something's not a tools area if it there are no tools (or even menus) in it. It's just a titlebar.

I take the tools area separator as proposed in the task to make sense as it's an element of internal design meant to help the structure of the whole application by singling out one of its semantic units.

On the level of the fluidity of design it works because it doesn't separate KWin out from everything else in doing so.

A titlebar separator doesn't do the two things just mentioned and I'd say it's is even somewhat orthogonal to the task because it's added externally with blindness to a specific app's design.

tl;dr: for T10201 I wouldn't add a separator when we only have the titlebar.

Is that configurable (Window decorations -> ^ Draw separator between titlebar and a window ?

filipf added a comment.Dec 4 2019, 1:26 PM

Is that configurable (Window decorations -> ^ Draw separator between titlebar and a window ?

No, the separator is always drawn.

So with the patch is it drawn when it is ticked in window decoration?

filipf added a comment.Dec 4 2019, 1:32 PM

So with the patch is it drawn when it is ticked in window decoration?

Yes. With the patch there is only a top separator on the sidebar, not between the titlebar and window.

Then you can turn on KWin's separator and get this:

TBH the top sidebar separator should ideally be disabled when KWin's separator is on.

manueljlin added a subscriber: manueljlin.EditedDec 4 2019, 10:11 PM

But that style of line / divider was going to be after the toolbars too, like kirigami apps, so removing it just would make that inconsistent later on. The divider needs to be inside the application itself to make this possible, instead of in the titlebar.

If it's possible, fixing the color scheme and making it configurable should probably be the way to go.

But that style of line / divider was going to be after the toolbars too, like kirigami apps, so removing it just would make that inconsistent later on. The divider needs to be inside the application itself to make this possible, instead of in the titlebar.

If it's possible, fixing the color scheme and making it configurable should probably be the way to go.

It's just wrong to locally draw a titlebar separator if there's a global option for it.

For consistency with the proposals see: D25728#571938

I can also say that after seeing Sierra Breeze Enhanced implementing most of what was decided there (only!) when the titlebar separator option is on that it's something that we should also do.

KWin's titlebar option essentially becomes a "tools area distinguisher": turns on the titlebar separator (which gets hidden when app sends a hint that it has a tool area that is will separate on its own) and the different tool area color as well. Everything is consistent and in accordance to user choice.

I though that would just affect every app's titlebar without actually checking if the app has a toolbar, etc etc and just add/remove the divider everywhere and be done with it. However, if the app can actually send a hint to Kwin to make it hide or show the divider depending if it has a toolbar or not (or maybe through a window / app rule), then it's great

I though that would just affect every app's titlebar without actually checking if the app has a toolbar, etc etc and just add/remove the divider everywhere and be done with it. However, if the app can actually send a hint to Kwin to make it hide or show the divider depending if it has a toolbar or not (or maybe through a window / app rule), then it's great

Exactly.

We can't rely 100% on KWin to draw the separator because with our proposed redesign, we want to have the separator below the menu or toolbar, if those are present. That means they would need to drawn by the app or the widget toolkit, not the window manager. That means that we can't have KWin always draw or not draw the titlebar. That means we need to have the separator drawn here.

However there are two caveats that I can accept:

  • Right now this is the only kind of window that gains a separator drawn by the content view itself, so we should either turn it off or add it to all other windows with only titlebars
  • We should make the content view not draw the titlebar when KWin is drawing it

Maybe what we should do is change the titlebar separator color in Breeze to be a pleasant dark gray like in the screenshot in @flipwise's latest comment instead of a garish blue, turn it on by default, and remove this here. Then for Plasma 6 (or whenever we do the proposed Breeze evolution) we can revisit that implementation and come up with something smarter.

Does that make sense?

With D27669, it's probably time to revisit drawing this separator at all. We should probably be relying on the widget and/or decoration style to do it.

This comment was removed by Codezela.
filipf added a comment.Apr 2 2020, 8:01 PM

Maybe what we should do is change the titlebar separator color in Breeze to be a pleasant dark gray like in the screenshot in @flipwise's latest comment instead of a garish blue, turn it on by default, and remove this here. Then for Plasma 6 (or whenever we do the proposed Breeze evolution) we can revisit that implementation and come up with something smarter.

So with the patch is it drawn when it is ticked in window decoration?

Yes. With the patch there is only a top separator on the sidebar, not between the titlebar and window.

Then you can turn on KWin's separator and get this:

TBH the top sidebar separator should ideally be disabled when KWin's separator is on.

Iam sry to ask this
I know it's not the right place to ask this
What is this icon themes you use
I will delete this comment soon
Sry again

Don't use Phabricator for these purposes in the future. There is #kde-chat@freenode or https://t.me/kdechat

The icon theme is: https://github.com/Nitrux/luv-icon-theme

ngraham added a subscriber: cblack.Apr 14 2020, 4:33 AM

With D27669, it's probably time to revisit drawing this separator at all. We should probably be relying on the widget and/or decoration style to do it.

^^ Ping @filipf @cblack

I guess the top separator should be removed altogether then? (As opposed to just shortening it)

filipf updated this revision to Diff 80582.Apr 19 2020, 7:04 PM

remove the separator instead of just shortening it

filipf retitled this revision from [Applet Configuration] Don't draw a full-on separator between the titlebar and window to [Applet Configuration] Don't draw a separator between the titlebar and window.Apr 19 2020, 7:05 PM
filipf edited the summary of this revision. (Show Details)
filipf edited the test plan for this revision. (Show Details)Apr 19 2020, 7:08 PM
filipf added a dependency: D27669: [kstyle] Tools area.
ngraham accepted this revision.Apr 19 2020, 7:15 PM
This revision is now accepted and ready to land.Apr 19 2020, 7:15 PM