Differential D27669 Diff 77312 kstyle/breezetoolsareamanager.cpp

Changeset View

Standalone View

View Options

kstyle/breezetoolsareamanager.cpp

  • This file was added.
1​#include "breezetoolsareamanager.h"
2​#include <QApplication>
3​#include <QObject>
4​#include <QWidget>
5​#include <QToolBar>
6​#include <QWindow>
7​#include <QMainWindow>
8​#include <QDebug>
9​
10​namespace Breeze {
11​ ToolsAreaManager::ToolsAreaManager(QObject *parent, Helper *helper) : QObject(parent), _helper(helper) {
12​
13​ }
14​
15​ ToolsAreaManager::~ToolsAreaManager() {
16​ for (auto entry : animationMap) {
hpereiradacostaUnsubmitted
Done

This is not needed. Since all animations are created as children of the ToolsAreaManager, they should be deleted automatically when this one is destroyed.
This way you can simply remove the class destructor altogether and rely on the default one (from QObject).

hpereiradacosta: This is not needed. Since all animations are created as children of the ToolsAreaManager, they…
17​ delete entry.foregroundColorAnimation;
18​ delete entry.backgroundColorAnimation;
19​ }
20​ }
21​
22​ void ToolsAreaManager::updateAnimations() {
23​ for (auto entry : animationMap) {
24​ entry.foregroundColorAnimation->setStartValue(_helper->titleBarTextColor(false));
25​ entry.foregroundColorAnimation->setEndValue(_helper->titleBarTextColor(true));
26​
27​ entry.backgroundColorAnimation->setStartValue(_helper->titleBarColor(false));
28​ entry.backgroundColorAnimation->setEndValue(_helper->titleBarColor(true));
29​ }
30​ }
31​
32​ void ToolsAreaManager::registerAnimation(QWidget *widget) {
33​ auto window = widget->window()->windowHandle();
34​ if (window && !animationMap.contains(window)) {
35​
36​ auto foregroundColorAnimation = new QVariantAnimation(this);
37​ connect(foregroundColorAnimation, &QVariantAnimation::valueChanged,
38​ this, &ToolsAreaManager::toolbarUpdated);
39​
40​ auto backgroundColorAnimation = new QVariantAnimation(this);
41​ connect(backgroundColorAnimation, &QVariantAnimation::valueChanged,
42​ this, &ToolsAreaManager::toolbarUpdated);
43​
44​ foregroundColorAnimation->setStartValue(_helper->titleBarTextColor(false));
45​ foregroundColorAnimation->setEndValue(_helper->titleBarTextColor(true));
46​
47​ backgroundColorAnimation->setStartValue(_helper->titleBarColor(false));
48​ backgroundColorAnimation->setEndValue(_helper->titleBarColor(true));
49​
50​ foregroundColorAnimation->setDuration(50);
51​ backgroundColorAnimation->setDuration(50);
52​
53​ animationMap[window] = ToolsAreaAnimation{
54​ foregroundColorAnimation,
55​ backgroundColorAnimation,
56​ window->isActive(),
57​ };
58​
59​ connect(window, &QWindow::activeChanged,
60​ this, [=]() {
61​ if (animationMap[window].foregroundColorAnimation.isNull() || animationMap[window].backgroundColorAnimation.isNull()) return;
62​
63​ auto prevActive = animationMap[window].prevActive;
64​ if (prevActive && !window->isActive()) {
65​ animationMap[window].foregroundColorAnimation->setDirection(QAbstractAnimation::Backward);
66​ animationMap[window].backgroundColorAnimation->setDirection(QAbstractAnimation::Backward);
67​
68​ animationMap[window].foregroundColorAnimation->start();
69​ animationMap[window].backgroundColorAnimation->start();
70​ } else if (!prevActive && window->isActive()) {
71​ animationMap[window].foregroundColorAnimation->setDirection(QAbstractAnimation::Forward);
72​ animationMap[window].backgroundColorAnimation->setDirection(QAbstractAnimation::Forward);
73​
74​ animationMap[window].foregroundColorAnimation->start();
75​ animationMap[window].backgroundColorAnimation->start();
76​ }
77​ animationMap[window].prevActive = window->isActive();
78​ });
79​
80​ }
81​ }
82​
83​ QColor ToolsAreaManager::foreground(const QWidget *widget) {
84​ auto window = widget->window()->windowHandle();
85​ if (window && animationMap.contains(window) && animationMap[window].foregroundColorAnimation) {
86​ return animationMap[window].foregroundColorAnimation->currentValue().value<QColor>();
87​ }
88​ return QColor();
89​ }
90​
91​ QColor ToolsAreaManager::background(const QWidget *widget) {
92​ auto window = widget->window()->windowHandle();
93​ if (window && animationMap.contains(window) && animationMap[window].backgroundColorAnimation) {
94​ return animationMap[window].backgroundColorAnimation->currentValue().value<QColor>();
95​ }
96​ return QColor();
97​ }
98​
hpereiradacostaUnsubmitted
Done

As far as I have followed the code you need an "unregisterWidget" for when widgets are unpolished and to remove windows from the map.
You also need a connection to the destroyed signal to cleanup the map when a widget is deleted, otherwise you will keep accumulating an ever growing map of dangling pointers.

hpereiradacosta: As far as I have followed the code you need an "unregisterWidget" for when widgets are…
99​
100​ void ToolsAreaManager::registerWidget(QWidget *widget)
101​ {
102​ if (QApplication::desktopFileName() == QStringLiteral("telegram-desktop.desktop")) return;
davidreUnsubmitted
Done

This needs at least a comment why we have this hack for telegram

davidre: This needs at least a comment why we have this hack for telegram
103​ auto window = qobject_cast<QMainWindow*> (widget);
104​ if (window) {
105​ connect(this, &ToolsAreaManager::toolbarUpdated,
106​ window, [this, window]() {
107​ if (_helper->toolsAreaHasContents(window)) {
108​ window->setContentsMargins(0,0,0,0);
109​ } else {
ngrahamUnsubmitted
Done

This seems to make the window content move vertically when the window loses focus

ngraham: This seems to make the window content move vertically when the window loses focus
cblackAuthorUnsubmitted
Done

Eh? What application do you see this behaviour in?

cblack: Eh? What application do you see this behaviour in?
ngrahamUnsubmitted
Done

All of them with anything besides a titlebar in their Tools area: Dolphin, Gwenview, Okular, Kate, Konsole...

ngraham: All of them with anything besides a titlebar in their Tools area: Dolphin, Gwenview, Okular…
ngrahamUnsubmitted
Done

ngraham: {F8168127}
cblackAuthorUnsubmitted
Done

That's odd—the margin shouldn't be shifting based off of window status. Will investigate.

cblack: That's odd—the margin shouldn't be shifting based off of window status. Will investigate.
cblackAuthorUnsubmitted
Done

I believe this is due to how it determines "should draw tools area" conflicting with your colourscheme.
When active, the window and the titlebar colours are different, thus causing the tools are to be drawn. When inactive, the window and the titlebar colours are the same, causing the tools area to not be drawn.

cblack: I believe this is due to how it determines "should draw tools area" conflicting with your…
hpereiradacostaUnsubmitted
Done

That's still needs fixing though. You cannot have things floating around depending on which color scheme is used. right ?
There are many color schemes of all types in the wild. The widget style must work for all.

hpereiradacosta: That's still needs fixing though. You cannot have things floating around depending on which…
cblackAuthorUnsubmitted
Done

This is intentional—if a window's titlebar and content colours are the same, the window is drawn as a homogeneous window instead of a window with a tools area.

cblack: This is intentional—if a window's titlebar and content colours are the same, the window is…
hpereiradacostaUnsubmitted
Done

I was not refering to the rendering of the separator, but rather to the fact that the widgets margin change between active and inactive. This should not happen (disregarding the color scheme).
All in all, whether you need to draw the separator or not you need to add the margin always (or follow David's suggestion)
Unless I misunderstood the actual issue reported by Nathan of course.

hpereiradacosta: I was not refering to the rendering of the separator, but rather to the fact that the widgets…
110​ window->setContentsMargins(0,1,0,0);
davidreUnsubmitted
Done

Maybe the decision whether to do this could only be made based on the colors of the active state so it's consistent with the inactive one.
Also this happens because the separator line? Do we actually need it? Currently there is no separator between titlebar and window and/or between toolbar and other window content?

davidre: Maybe the decision whether to do this could only be made based on the colors of the active…
111​ }
112​ });
113​ }
114​ connect(this, &ToolsAreaManager::toolbarUpdated,
115​ widget, [widget]() {
116​ widget->update();
117​ });
118​ auto toolbar = qobject_cast<QToolBar*>(widget);
119​ if (toolbar) {
120​ connect(toolbar, &QToolBar::visibilityChanged,
121​ this, [this]() {
hpereiradacostaUnsubmitted
Done

the this-> here are all unnecessary.

hpereiradacosta: the this-> here are all unnecessary.
122​ emit this->toolbarUpdated();
123​ });
124​ connect(toolbar, &QToolBar::orientationChanged,
125​ this, [this]() {
126​ emit this->toolbarUpdated();
127​ });
128​ connect(toolbar, &QToolBar::topLevelChanged,
ndavisUnsubmitted
Not Done

When I run GDB on oxygen-demo5, sometimes it tells me that oxygen-demo5 crashes on this line. I'm not sure how to make it happen reliably, but it usually involves changing QStyles with the Style combobox or color schemes with the Color Scheme combobox.

ndavis: When I run GDB on `oxygen-demo5`, sometimes it tells me that `oxygen-demo5` crashes on this…
ndavisUnsubmitted
Not Done

Now I can consistently get oxygen-demo5 to crash on this line by switching to Fusion and then Oxygen.

ndavis: Now I can consistently get oxygen-demo5 to crash on this line by switching to Fusion and then…
ndavisUnsubmitted
Not Done

I misclicked. It's this line with _helper->_invalidateCachedRects = true;

ndavis: I misclicked. It's this line with `_helper->_invalidateCachedRects = true;`
129​ this, [this]() {
130​ emit this->toolbarUpdated();
131​ });
132​ }
133​ connect(widget, &QObject::destroyed,
134​ this, [this, widget]() {
hpereiradacostaUnsubmitted
Done

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: Did you check that this chunk of code (changing the contents margins of the QMainWindow) has…
cblackAuthorUnsubmitted
Done

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.

cblack: This only triggers when the window has no items in the tools area—it's used in conjunction with…
hpereiradacostaUnsubmitted
Done

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

hpereiradacosta: Clear enough ! Sorry for the noise ! (and not having read the code with enough attention).
135​ this->unregisterWidget(widget);
136​ });
137​ registerAnimation(widget);
138​ _registeredWidgets << widget;
139​ emit this->toolbarUpdated();
140​ }
hpereiradacostaUnsubmitted
Done

more unnecessary "this->"
Please try to remove them all.

hpereiradacosta: more unnecessary "this->" Please try to remove them all.
141​
142​ void ToolsAreaManager::unregisterWidget(QWidget *widget)
143​ {
144​ _registeredWidgets.removeAll(widget);
145​ QList<QWindow*> toRemove;
146​ for (auto window : animationMap.keys()) {
147​ bool hasWidget = false;
hpereiradacostaUnsubmitted
Done

you can break here once one widget is found. no need to loop over the remaining widgets.
alternatively you can use std::any_of or (better) std::none_of

hpereiradacosta: you can break here once one widget is found. no need to loop over the remaining widgets.
148​ if (std::none_of(_registeredWidgets.begin(), _registeredWidgets.end(), [window](QWidget *widget) {
149​ return window == widget->window()->windowHandle();
150​ })) {
151​ delete animationMap[window].foregroundColorAnimation;
152​ delete animationMap[window].backgroundColorAnimation;
153​ toRemove << window;
154​ }
155​ }
156​ for (auto entry : toRemove) {
157​ animationMap.remove(entry);
158​ }
159​ }
160​}
hpereiradacostaUnsubmitted
Done

you should also remove the widget from widgetsWithPaletteForToolsAreaSet

hpereiradacosta: you should also remove the widget from widgetsWithPaletteForToolsAreaSet
161​No newline at end of file
hpereiradacostaUnsubmitted
Done

Unused

hpereiradacosta: Unused