Changeset View
Standalone View
src/kmessagewidget.cpp
Show All 26 Lines | |||||
27 | #include <QHBoxLayout> | 27 | #include <QHBoxLayout> | ||
28 | #include <QLabel> | 28 | #include <QLabel> | ||
29 | #include <QPainter> | 29 | #include <QPainter> | ||
30 | #include <QShowEvent> | 30 | #include <QShowEvent> | ||
31 | #include <QTimeLine> | 31 | #include <QTimeLine> | ||
32 | #include <QToolButton> | 32 | #include <QToolButton> | ||
33 | #include <QStyle> | 33 | #include <QStyle> | ||
34 | 34 | | |||
35 | #include <QStandardPaths> | ||||
36 | #include <QSettings> | ||||
37 | #include <QDebug> | ||||
38 | | ||||
39 | //--------------------------------------------------------------------- | ||||
40 | // KGlobalSettings | ||||
41 | // A basic interface to read the global KDE settings store (kdeglobals) | ||||
42 | //--------------------------------------------------------------------- | ||||
43 | class KGlobalSettings | ||||
44 | { | ||||
45 | public: | ||||
46 | KGlobalSettings(const QString &initialGroup = QString()) | ||||
47 | { | ||||
48 | const QString kdeGlobals = QStandardPaths::locate(QStandardPaths::ConfigLocation, QStringLiteral("kdeglobals")); | ||||
49 | if (!kdeGlobals.isEmpty()) { | ||||
50 | m_settings = new QSettings(kdeGlobals, QSettings::IniFormat); | ||||
51 | } | ||||
52 | if (m_settings && !initialGroup.isEmpty()){ | ||||
53 | if (m_settings->childGroups().contains(initialGroup)) { | ||||
54 | m_settings->beginGroup(initialGroup); | ||||
55 | } else { | ||||
56 | delete m_settings; | ||||
57 | m_settings = nullptr; | ||||
58 | } | ||||
59 | } | ||||
60 | } | ||||
61 | | ||||
62 | virtual ~KGlobalSettings() | ||||
63 | { | ||||
64 | delete m_settings; | ||||
65 | } | ||||
66 | | ||||
67 | QColor readRGBRaw(const QString &key, QColor value = QColor()) | ||||
68 | { | ||||
69 | if (m_settings && m_settings->contains(key)) { | ||||
70 | QStringList rgb = m_settings->value(key).toStringList(); | ||||
71 | if (rgb.size() >= 3) { | ||||
72 | QColor newValue(rgb.at(0).toInt(), rgb.at(1).toInt(), rgb.at(2).toInt()); | ||||
73 | if (newValue.isValid()) { | ||||
74 | value = newValue; | ||||
75 | } | ||||
76 | } | ||||
77 | } | ||||
78 | return value; | ||||
79 | } | ||||
80 | | ||||
81 | QColor readRGB(const QString &key, QColor value = QColor()) | ||||
82 | { | ||||
83 | // | ||||
84 | if (qEnvironmentVariableIsSet("KMESSAGEWIDGET_BACKGROUND_BRIGHTNESS_MATCHED")) { | ||||
85 | return readRGBMatched(key, value); | ||||
86 | } else { | ||||
87 | return readRGBRaw(key, value); | ||||
88 | } | ||||
89 | } | ||||
90 | | ||||
91 | // read an RGB triplet from <key> in the currently active group, or return | ||||
92 | // the default <value> if the key doesn't exist. The returned RGB value is | ||||
93 | // corrected so that its brightness is as close as possible to the brightness | ||||
94 | // of the default colour. | ||||
95 | QColor readRGBMatched(const QString &key, QColor value = QColor()) | ||||
96 | { | ||||
97 | qreal defaultBrightness = value.isValid() ? psychoMetricBrightness(value) : -1; | ||||
98 | qWarning() << "Default intensity for key" << key << ":" << defaultBrightness; | ||||
99 | if (m_settings && m_settings->contains(key)) { | ||||
100 | QStringList rgb = m_settings->value(key).toStringList(); | ||||
101 | if (rgb.size() >= 3) { | ||||
102 | QColor newValue(rgb.at(0).toInt(), rgb.at(1).toInt(), rgb.at(2).toInt()); | ||||
103 | if (newValue.isValid()) { | ||||
104 | qreal newBrightness = psychoMetricBrightness(newValue); | ||||
105 | if (defaultBrightness >= 0 && newBrightness > 0) { | ||||
106 | // determine the largest factor by which we can scale the RGB | ||||
107 | // triplet to approach the reference brightness as much as possible. | ||||
108 | qreal brightnessScaleFactor = defaultBrightness / newBrightness; | ||||
109 | qreal maxRGB = newValue.redF(); | ||||
110 | if (newValue.greenF() > maxRGB) { | ||||
111 | maxRGB = newValue.greenF(); | ||||
112 | } | ||||
113 | if (newValue.blueF() > maxRGB) { | ||||
114 | maxRGB = newValue.blueF(); | ||||
115 | } | ||||
116 | if (maxRGB * brightnessScaleFactor > 1) { | ||||
117 | brightnessScaleFactor = 1 / maxRGB; | ||||
118 | } | ||||
119 | qWarning() << "brightness scaling:" << newBrightness << ":" | ||||
120 | << brightnessScaleFactor << "*" << newValue.redF() << newValue.greenF() << newValue.blueF(); | ||||
121 | value.setRgbF(newValue.redF() * brightnessScaleFactor | ||||
122 | , newValue.greenF() * brightnessScaleFactor | ||||
123 | , newValue.blueF() * brightnessScaleFactor); | ||||
124 | } else { | ||||
125 | value = newValue; | ||||
126 | } | ||||
127 | qWarning() << "New intensity:" << psychoMetricBrightness(value); | ||||
128 | } | ||||
129 | } | ||||
130 | } | ||||
131 | return value; | ||||
132 | } | ||||
133 | | ||||
134 | private: | ||||
135 | // calculate the perceived (psycho-metric) brightness for an RGB triplet | ||||
136 | qreal psychoMetricBrightness(QColor rgb) | ||||
137 | { | ||||
138 | return 0.299 * rgb.redF() + 0.586 * rgb.greenF() + 0.115 * rgb.blueF(); | ||||
139 | } | ||||
140 | | ||||
141 | QSettings *m_settings = nullptr; | ||||
142 | }; | ||||
143 | | ||||
35 | //--------------------------------------------------------------------- | 144 | //--------------------------------------------------------------------- | ||
36 | // KMessageWidgetPrivate | 145 | // KMessageWidgetPrivate | ||
37 | //--------------------------------------------------------------------- | 146 | //--------------------------------------------------------------------- | ||
38 | class KMessageWidgetPrivate | 147 | class KMessageWidgetPrivate | ||
39 | { | 148 | { | ||
40 | public: | 149 | public: | ||
41 | void init(KMessageWidget *); | 150 | void init(KMessageWidget *); | ||
42 | 151 | | |||
▲ Show 20 Lines • Show All 209 Lines • ▼ Show 20 Line(s) | |||||
252 | KMessageWidget::MessageType KMessageWidget::messageType() const | 361 | KMessageWidget::MessageType KMessageWidget::messageType() const | ||
253 | { | 362 | { | ||
254 | return d->messageType; | 363 | return d->messageType; | ||
255 | } | 364 | } | ||
256 | 365 | | |||
257 | void KMessageWidget::setMessageType(KMessageWidget::MessageType type) | 366 | void KMessageWidget::setMessageType(KMessageWidget::MessageType type) | ||
258 | { | 367 | { | ||
259 | d->messageType = type; | 368 | d->messageType = type; | ||
260 | QColor bgBaseColor; | 369 | QColor bgBaseColor; | ||
cfeck: missing `if (rgb.size() >= 3)` | |||||
370 | const QPalette palette = QGuiApplication::palette(); | ||||
261 | 371 | | |||
Using the global application palette was deliberate. Try your change and then suspend a terminal window that has a dark background when you're using Breeze light. Local widgets may set a palette that's incompatible with some of the colors we're forced to hardcode due to KWidgetsAddons being a Tier 1 framework, or may set a palette that's incomplete and results in unreadable text. It's much safer to just use the app's own palette. ngraham: Using the global application palette was deliberate. Try your change and then suspend a… | |||||
262 | // We have to hardcode colors here because KWidgetsAddons is a tier 1 framework | 372 | // We cannot use KColorScheme here because KWidgetsAddons is a tier 1 framework | ||
cfeck: `>= 4` | |||||
263 | // and therefore can't depend on any other KDE Frameworks | 373 | // and therefore can't depend on any other KDE Frameworks. We thus try to get | ||
264 | // The following RGB color values come from the "default" scheme in kcolorscheme.cpp | 374 | // the colours that interest us directly from the global settings store using QSettings. | ||
375 | // It that fails we use hardcoded colours, or the highlight colour (for Information). | ||||
ngraham: This commented-out line and its comments should be removed. | |||||
ngraham: Never mind, you were too fast for me. :) | |||||
376 | // The hardcoded RGB color bgBaseColors come from the "default" scheme in kcolorscheme.cpp | ||||
377 | KGlobalSettings settings(QStringLiteral("Colors:Window")); | ||||
265 | switch (type) { | 378 | switch (type) { | ||
266 | case Positive: | 379 | case Positive: | ||
cfeck: missing space before `?` | |||||
267 | bgBaseColor.setRgb(39, 174, 96); // Window: ForegroundPositive | 380 | bgBaseColor = settings.readRGB(QStringLiteral("ForegroundPositive"), QColor(39, 174, 96)); | ||
I can't find the definition of ForegroundPositive in https://cgit.kde.org/kcmutils.git/tree/src/kdeglobals.kcfg am i missing something? aacid: I can't find the definition of ForegroundPositive in https://cgit.kde.org/kcmutils. | |||||
268 | break; | 381 | break; | ||
269 | case Information: | 382 | case Information: | ||
270 | bgBaseColor.setRgb(61, 174, 233); // Window: ForegroundActive | 383 | bgBaseColor = settings.readRGB(QStringLiteral("ForegroundActive"), palette.highlight().color()); | ||
"as before" not needed. Really, @cfeck is right and the entire comment isn't needed. ngraham: "as before" not needed. Really, @cfeck is right and the entire comment isn't needed. | |||||
271 | break; | 384 | break; | ||
272 | case Warning: | 385 | case Warning: | ||
273 | bgBaseColor.setRgb(246, 116, 0); // Window: ForegroundNeutral | 386 | bgBaseColor = settings.readRGB(QStringLiteral("ForegroundNeutral"), QColor(246, 116, 0)); | ||
274 | break; | 387 | break; | ||
275 | case Error: | 388 | case Error: | ||
276 | bgBaseColor.setRgb(218, 68, 83); // Window: ForegroundNegative | 389 | bgBaseColor = settings.readRGB(QStringLiteral("ForegroundNegative"), QColor(218, 68, 83)); | ||
277 | break; | 390 | break; | ||
278 | } | 391 | } | ||
279 | const qreal bgBaseColorAlpha = 0.2; | 392 | const qreal bgBaseColorAlpha = 0.2; | ||
280 | bgBaseColor.setAlphaF(bgBaseColorAlpha); | 393 | bgBaseColor.setAlphaF(bgBaseColorAlpha); | ||
281 | 394 | | |||
I don't think all of this complicated code is necessary. The theme itself is supposed to ensure readability with the colors that it uses. Also, this results in a slight regression for the Breeze Dark theme: the Information widget doesn't get enough alpha and is too bright, worsening readability compared to the status quo. ngraham: I don't think all of this complicated code is necessary. The theme itself is supposed to ensure… | |||||
Also, commits should be atomic; even if we want to do this, it should be in another patch since it represents a separate conceptual change compared to the status quo, as opposed to simply a bugfix or missing feature. ngraham: Also, commits should be atomic; even if we want to do this, it should be in another patch since… | |||||
What, the alpha tweak? Do you really want to split this up in a 1st commit that makes things worse for some followed by one that make things better than the status quo? We'll see about that later, any splitting up is going to me the WIP and review more complicated (if not I could also submit 5 different reviews, 1 introducing the basic QSettings code, and one for using it for each of the message types...) rjvbb: What, the alpha tweak? Do you really want to split this up in a 1st commit that makes things… | |||||
The issue I have with using these colours is that they are foreground colours, to be used against one of 2 or 3 background colours. *That* is what the theme can be expected to ensure. There is however no obstacle whatsoever to introducing a few new colour categories, specifically for this use. Only then can you blame the theme if things don't work out. I can modify (and maybe simplify) the alpha calculation of course. Here's what my theme gives with the tweaked alpha. Without that, information messages would be a darker red than error messages. That would be wrong, but I don't see how there's anything wrong with using a dark red for active text and a brighter red for negative. Red on grey works very nicely for text that should draw attention. rjvbb: The issue I have with using these colours is that they are foreground colours, to be used… | |||||
That's your theme's problem. You used almost the same color for Negative and Active. Isn't the whole point of this to respect the user's theme? If the user chooses theme colors that are problematic, we can't help that. Either way, we certainly can't include a workaround that's tailored to a problem introduced by your specific choice of theme colors. ngraham: That's your theme's problem. You used almost the same color for Negative and Active. Isn't the… | |||||
282 | const QPalette palette = QGuiApplication::palette(); | | |||
ngraham: Same | |||||
283 | const QColor windowColor = palette.window().color(); | 395 | const QColor windowColor = palette.window().color(); | ||
284 | const QColor textColor = palette.text().color(); | 396 | // prefer the theme's normal foreground colour for text for more reliable readability with dark themes | ||
397 | const QColor textColor = settings.readRGBRaw(QStringLiteral("ForegroundNormal"), palette.text().color()); | ||||
285 | const QColor border = bgBaseColor; | 398 | const QColor border = bgBaseColor; | ||
286 | 399 | | |||
287 | // Generate a final background color from overlaying bgBaseColor over windowColor | 400 | // Generate a final background color from overlaying bgBaseColor over windowColor | ||
288 | const int newRed = (bgBaseColor.red() * bgBaseColorAlpha) + (windowColor.red() * (1 - bgBaseColorAlpha)); | 401 | const int newRed = (bgBaseColor.red() * bgBaseColorAlpha) + (windowColor.red() * (1 - bgBaseColorAlpha)); | ||
289 | const int newGreen = (bgBaseColor.green() * bgBaseColorAlpha) + (windowColor.green() * (1 - bgBaseColorAlpha)); | 402 | const int newGreen = (bgBaseColor.green() * bgBaseColorAlpha) + (windowColor.green() * (1 - bgBaseColorAlpha)); | ||
290 | const int newBlue = (bgBaseColor.blue() * bgBaseColorAlpha) + (windowColor.blue() * (1 - bgBaseColorAlpha)); | 403 | const int newBlue = (bgBaseColor.blue() * bgBaseColorAlpha) + (windowColor.blue() * (1 - bgBaseColorAlpha)); | ||
291 | 404 | | |||
292 | const QColor bgFinalColor = QColor(newRed, newGreen, newBlue); | 405 | const QColor bgFinalColor = QColor(newRed, newGreen, newBlue); | ||
▲ Show 20 Lines • Show All 203 Lines • Show Last 20 Lines |
missing if (rgb.size() >= 3)