[kstyle] Tools area
Needs ReviewPublic

Authored by cblack on Feb 26 2020, 2:02 AM.

Details

Summary

There is now a tools area.

Test Plan

(no border drawn when titlebar and content colours are the same)

Diff Detail

Repository
R31 Breeze
Branch
cblack/toolsarea
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24388
Build 24406: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

Address feedback

cblack added inline comments.Fri, Mar 20, 1:17 AM
kstyle/breeze.h
104 ↗(On Diff #77334)

With:


Without:

cblack updated this revision to Diff 78065.Fri, Mar 20, 1:48 AM

Use better heuristic for determining if a widget has the correct palette set

Nice, the colors are better now. I still see a difference in animation speed when the titlebar and toolbar change color though. It's especially visible with the current Breeze color scheme.

cblack added a comment.EditedFri, Mar 20, 2:54 AM

Nice, the colors are better now. I still see a difference in animation speed when the titlebar and toolbar change color though. It's especially visible with the current Breeze color scheme.

This is only an issue on X11. On Wayland, the titlebar and window contents are properly synchronized.

Unfortunately there's not much that can be done here.

Some colorschemes do have transparent titlebars, do you plan to support transparency or leave it as is?

To support apps that have another color schemes than the global one look at the property KColoSchemeManager sets: https://cgit.kde.org/kconfigwidgets.git/tree/src/kcolorschememanager.cpp#n42
Otherwise it looks like this:

niccolove added a comment.EditedFri, Mar 20, 8:50 AM

If possible, I think that supporting toparea transparency with those colorscheme would be a great feature for people who would like plasma to look like this (see below), as they would only need to change colorscheme.


My mockup for a slighly transparent toolsarea (on the right, that's what I'd like to be the default, but I know, I know.):

Currently it does not blur behind it, but that could be implemented.

gvgeo removed a subscriber: gvgeo.Fri, Mar 20, 4:33 PM
hpereiradacosta added a comment.EditedSat, Mar 21, 8:23 PM

Hi Carson,
Thanks for the updated patch and screenshot.
First I agree that the new (latest) checks on whether the toolbar palette was changed or not are much more elegant and just as efficient as the previous implementation.
Second, some more comments below.

kstyle/breeze.h
104 ↗(On Diff #77334)

Hi,
thanks for posting the screenshots. If I understand right this is really an issue with the bottom margin only, and in particular with respect to the separator. In that case I agree this is part of this patch.
I don't think the solution is the right one though. First, changing the metric will also affect widget rendering when

  • the effect is disabled (and I don't think it should in that case)
  • when there is no separator line (and IMO I don't think the extra margin is needed in that case either).
  • when toolbars are located elsewhere (on the sides of the main window for instance).

Second, it affects all sides, whereas only the bottom one needs changed.
i would rather change the "contentsmargin" of the toolbars when necessary, while leaving the metrics unchanged. This could be done whenever you check that a given toolbar is in the toolarea.

If you insist on changing the margins on all toolbars, all sides, disregarding their location and disregarding whether the effect is enabled or not, then this is a change orthogonal (and of much broader scope) to this patch and must go to a different commit.

kstyle/breezetoolsareamanager.cpp
134

Did you check that this chunk of code (changing the contents margins of the QMainWindow) has any effect ? I had my doubt so I changed the margin to 100 instead of 1, added a printout to make sure that this line of code is called, then ran dolphin and
1/ the code is indeed called
2/ it changes nothing, on dolphin, okular, ...
if you can confirm, then just drop it.

(in fact, I don't understand the login in the first place: why would you need to add one pixel on top of everything in the QMainWindow ?)

hpereiradacosta added a comment.EditedSat, Mar 21, 8:40 PM

Nice, the colors are better now. I still see a difference in animation speed when the titlebar and toolbar change color though. It's especially visible with the current Breeze color scheme.

Here at least (also X11) not only is the window decoration out of sync with the toolarea, but when you have both a menubar and a toolbar (as in e.g. okular), both are also out of sync. Can you double-check, confirm ?
I think it is because rendering the background is expensive (especially in case of animation) and both widgets are not rendered at the exact same time.
one solution could be to render the background directly on the mainwindow (leaving the toolbar and the menubars transparent, as they were before). I have not investigated further how difficult this would be to implement though (if even possible)

Nice, the colors are better now. I still see a difference in animation speed when the titlebar and toolbar change color though. It's especially visible with the current Breeze color scheme.

Here at least (also X11) not only is the window decoration out of sync with the toolarea, but when you have both a menubar and a toolbar (as in e.g. okular), both are also out of sync. Can you double-check, confirm ?
I think it is because rendering the background is expensive (especially in case of animation) and both widgets are not rendered at the exact same time.
one solution could be to render the background directly on the mainwindow (leaving the toolbar and the menubars transparent, as they were before). I have not investigated further how difficult this would be to implement though (if even possible)

I could move the rendering of the background and border of the tools area into the window's background that's currently only utilised for drawing a border without tools area.

cblack added inline comments.Sat, Mar 21, 8:45 PM
kstyle/breezetoolsareamanager.cpp
134

This only triggers when the window has no items in the tools area—it's used in conjunction with line 878 of breezestyle.cpp in order to render a border at the top of the window.

kstyle/breezetoolsareamanager.cpp
134

Clear enough ! Sorry for the noise ! (and not having read the code with enough attention).

kstyle/breeze.h
104 ↗(On Diff #77334)

Sorry for the many postings, I had another unrelated comment on these screeshot:
I find it strange that the toolarea separator color in this screenshot is brighter than the toolbar separators (the vertical lines between tool buttons).
IMO it would make more sense to have both have the exact same color, since they fullfil the same role (separating ...)

cblack added inline comments.Sat, Mar 21, 8:51 PM
kstyle/breeze.h
104 ↗(On Diff #77334)

The tools area's separator has to separate two large areas of the window and thus should be stronger than the separators that only have to separate borderless and backgroundless buttons.

kstyle/breeze.h
104 ↗(On Diff #77334)

As I said, to me at least it does not look so good in the two screenshots you posted above.
In general I think it is not a good idea to arbitrarily multiply the various shades/colors used in a single ui, to fullfil the same role. Beside, the two areas in questions have a different background already which in itself constitutes a strong separator.
Anyway I'll let other VDG members comments/decide on this.
Just posting my opinion.
Not a show stopper on my side

Regarding the Tools Area separator color being different:

I agree with Hugo, and have voiced this concern before. It's also not consistent with the mockups and IMO the presented justification demonstrates why the different color isn't needed: the Tools Area's background color is already different, which aids in the visual separation. IMO the separator color doesn't need to be changed.

cblack updated this revision to Diff 78194.Sat, Mar 21, 10:05 PM

Render tools area background in drawWidgetPrimitive

cblack updated this revision to Diff 78196.Sun, Mar 22, 12:50 AM

Check for presence of KDE_COLOR_SCHEME_PATH and utilize it if set

cblack updated this revision to Diff 78198.Sun, Mar 22, 1:11 AM
cblack marked 8 inline comments as done.

Specifically set bottom margin on bottommost toolbar instead of using a metric

cblack updated this revision to Diff 78199.Sun, Mar 22, 1:42 AM

Address some feedback and fix issues

davidre added inline comments.Sun, Mar 22, 11:04 AM
kstyle/breezehelper.cpp
93

I don't think you need such a big if. Loading a kconfig with empty string will load kdeglobals

cblack updated this revision to Diff 78303.Mon, Mar 23, 6:12 PM
cblack marked an inline comment as done.

Drop giant if

cblack updated this revision to Diff 78306.Mon, Mar 23, 7:08 PM

Use explicit setting instead of colour palette heuristics

Hi Carson,
Thanks for the update. At first glance this all look pretty good. I should be able to do some more in depth testing and code review by the end of tomorrow.
Hugo

I finally got some time to look at the code. Some minor comments below (compiler warnings)
Also there is a problem with menubar text color when option is disabled. Here at least it is still set to the decoration color, leading to invisible text with default breeze color scheme.
See:


kstyle/breezehelper.cpp
53

compiler complains about wrong initialization order

kstyle/breezestyle.cpp
4939

same remark about option being unused

cblack updated this revision to Diff 78505.Wed, Mar 25, 10:21 PM

Address feedback

cblack marked 2 inline comments as done.Wed, Mar 25, 10:22 PM
hpereiradacosta accepted this revision.Wed, Mar 25, 10:42 PM

All good, as far as I am concerned. Thanks !

This revision is now accepted and ready to land.Wed, Mar 25, 10:42 PM

So I'm still having an issue with the feature um, not working. :( I have it turned on in the Breeze settings but it behaves as if off; that is to say, I see no different appearance compared to the status quo.

Is anyone else experiencing this, or just me?

So I'm still having an issue with the feature um, not working. :( I have it turned on in the Breeze settings but it behaves as if off; that is to say, I see no different appearance compared to the status quo.

Is anyone else experiencing this, or just me?

It doesn't work for me either. I tested with Dolphin and Kate.

So I'm still having an issue with the feature um, not working. :( I have it turned on in the Breeze settings but it behaves as if off; that is to say, I see no different appearance compared to the status quo.

Is anyone else experiencing this, or just me?

... it certainly is working here.

So ... can't help you (not to say that there isn't any problem. just that it is not completely trivial to find)

I use a kdesrc-build environment. My process was this:

cd breeze
arc patch D27669
ninja -C "$( pwd | sed 's:/src/:/build/:' )" install  #I use this through a script named kdesrc-ninja and it works well
source ~/kde/.setup-env  #This makes the terminal session use the kdesrc-build environment
dolphin

That white combo box looks a little nasty against the dark gray background. I wonder if there's anything that can be done about it.

davidre requested changes to this revision.Thu, Mar 26, 9:01 AM
davidre added inline comments.
kstyle/breezehelper.cpp
1785

This check is the problem. If "Use Theme's default border size" is checked (the default) then no entry will be written in the config file because the defaults are used. This is for`BorderSizeAuto` "true" and for BorderSize "Normal" (Note however that because of the the BorderSizeAuto the default borders of the deco are used and not normal sized borders)

This revision now requires changes to proceed.Thu, Mar 26, 9:01 AM

Some observation:

    • Performance in KDevelop is horrible (First startup, opening files, switching between file tabs...)
    • It also colors the toolbars of mdi windows
  • It also draws the line around some views double, see above screenshot and
  • Why are we drawing a separator if the window has no toolbar?

Can confirm, if I override the default border setting and change it to "no borders", the appearance looks right again. So I guess we need to handle cases where the window decoration theme's default border setting is "no borders".

It might also be nice to mention in the config UI that the appearance change only takes effect when using No Borders. The text could be something like "Visually merge toolbars and menubars into window titlebar when using no window borders"

  • Why are we drawing a separator if the window has no toolbar?

Because this looks really good :)

cblack updated this revision to Diff 78565.Thu, Mar 26, 4:17 PM

Account for the applications that think it's semantically correct to put a main window in a dock widget

cblack updated this revision to Diff 78567.Thu, Mar 26, 4:23 PM

Assume there's no borders when there's not a BorderSize entry

cblack marked an inline comment as done.Thu, Mar 26, 4:29 PM
kstyle/breezehelper.cpp
1688

mmm shouldn't you test on parent rather than widget here ?

cblack updated this revision to Diff 78572.Thu, Mar 26, 4:31 PM
cblack marked an inline comment as done.

Fix error

Assume there's no borders when there's not a BorderSize entry

That's totally not correct. The default value when there's no BorderSize entry is "Normal" . This patch is now also enabled when the user manually selects "Normal". The correct thing is to check first whether BorderSizeAuto is enabled (default true) . If it's not enabled you can read BorderSize (default "Normal") otherwise check what the preference the decoration sets. I didn't check yet where that value is defined

Assume there's no borders when there's not a BorderSize entry

That's totally not correct. The default value when there's no BorderSize entry is "Normal" . This patch is now also enabled when the user manually selects "Normal". The correct thing is to check first whether BorderSizeAuto is enabled (default true) . If it's not enabled you can read BorderSize (default "Normal") otherwise check what the preference the decoration sets. I didn't check yet where that value is defined

Took a look: it's not defined anywhere; it's up to the decorations to do whatever they feel like.

Yes, the decoration theme determines its own border value. For Breeze it's "No borders", but we can't assume that.

With the latest version of the patch, I still see this hilarious visual bug:

two videos showing the performance regression (on the left current breeze, on the right with this patch applied)

cblack updated this revision to Diff 78577.Thu, Mar 26, 5:01 PM

Don't include toolbars in MDI/dock widgets for the tools area calculation

For me it only merges the menus with the window decoration, not the toolbar :/

Can can confirm. Toolbars are no longer considered to be a part of the Tools area in all apps with the latest version of the patch.

cblack updated this revision to Diff 78664.Fri, Mar 27, 3:40 PM

Fix bad toolbar heuristic

Couple of things:

  • There's strong consensus in the VDG that we should go with a standard separator line for now, and eventually make it consistently darker if we decide it's not dark enough.
  • The above mentioned colorscheme with transparent titlebar still does not work; this is not a blocker and I've already contacted @cblack who told me they couldn't do it, but I'll still mention it in case somebody else knows why.
cblack updated this revision to Diff 78667.Fri, Mar 27, 3:53 PM

Address issues

cblack updated this revision to Diff 78754.Sat, Mar 28, 7:46 PM

Optimize toolbar rect function