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 | | ||||
35 | //--------------------------------------------------------------------- | 38 | //--------------------------------------------------------------------- | ||
36 | // KMessageWidgetPrivate | 39 | // KMessageWidgetPrivate | ||
37 | //--------------------------------------------------------------------- | 40 | //--------------------------------------------------------------------- | ||
38 | class KMessageWidgetPrivate | 41 | class KMessageWidgetPrivate | ||
39 | { | 42 | { | ||
40 | public: | 43 | public: | ||
41 | void init(KMessageWidget *); | 44 | void init(KMessageWidget *); | ||
42 | 45 | | |||
▲ Show 20 Lines • Show All 206 Lines • ▼ Show 20 Line(s) | 250 | { | |||
249 | updateGeometry(); | 252 | updateGeometry(); | ||
250 | } | 253 | } | ||
251 | 254 | | |||
252 | KMessageWidget::MessageType KMessageWidget::messageType() const | 255 | KMessageWidget::MessageType KMessageWidget::messageType() const | ||
253 | { | 256 | { | ||
254 | return d->messageType; | 257 | return d->messageType; | ||
255 | } | 258 | } | ||
256 | 259 | | |||
260 | static QColor qColorFromSettings(const QSettings *settings, const QString &key) | ||||
261 | { | ||||
262 | QStringList rgb = settings->value(key).toStringList(); | ||||
263 | return QColor(rgb.at(0).toInt(), rgb.at(1).toInt(), rgb.at(2).toInt()); | ||||
cfeck: missing `if (rgb.size() >= 3)` | |||||
264 | } | ||||
265 | | ||||
257 | void KMessageWidget::setMessageType(KMessageWidget::MessageType type) | 266 | void KMessageWidget::setMessageType(KMessageWidget::MessageType type) | ||
cfeck: `>= 4` | |||||
258 | { | 267 | { | ||
259 | d->messageType = type; | 268 | d->messageType = type; | ||
260 | QColor bgBaseColor; | 269 | QColor bgBaseColor; | ||
270 | const QPalette palette = QGuiApplication::palette(); | ||||
271 | qreal bgBaseColorAlpha = 0.2; | ||||
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… | |||||
272 | const QColor windowColor = palette.window().color(); | ||||
261 | 273 | | |||
262 | // We have to hardcode colors here because KWidgetsAddons is a tier 1 framework | 274 | // We cannot use KColorScheme here because KWidgetsAddons is a tier 1 framework | ||
263 | // and therefore can't depend on any other KDE Frameworks | 275 | // and therefore can't depend on any other KDE Frameworks. We thus try to get | ||
ngraham: This commented-out line and its comments should be removed. | |||||
ngraham: Never mind, you were too fast for me. :) | |||||
264 | // The following RGB color values come from the "default" scheme in kcolorscheme.cpp | 276 | // the colours that interest us directly from the global settings store using QSettings. | ||
277 | // It that fails we use hardcoded colours, or the highlight colour (for Information). | ||||
278 | // The hardcoded RGB color values come from the "default" scheme in kcolorscheme.cpp | ||||
279 | const QString kdeGlobals = QStandardPaths::locate(QStandardPaths::ConfigLocation, QStringLiteral("kdeglobals")); | ||||
280 | const QString themeGroup = QStringLiteral("Colors:Window"); | ||||
281 | QSettings *settings = !kdeGlobals.isEmpty()? new QSettings(kdeGlobals, QSettings::IniFormat) : nullptr; | ||||
cfeck: missing space before `?` | |||||
282 | if (settings && settings->childGroups().contains(themeGroup)) { | ||||
283 | settings->beginGroup(themeGroup); | ||||
284 | } else { | ||||
285 | delete settings; | ||||
286 | settings = nullptr; | ||||
287 | } | ||||
265 | switch (type) { | 288 | switch (type) { | ||
266 | case Positive: | 289 | case Positive: | ||
290 | if (settings && settings->contains(QStringLiteral("ForegroundPositive"))) { | ||||
291 | bgBaseColor = qColorFromSettings(settings, QStringLiteral("ForegroundPositive")); | ||||
292 | } else { | ||||
267 | bgBaseColor.setRgb(39, 174, 96); // Window: ForegroundPositive | 293 | bgBaseColor.setRgb(39, 174, 96); // Window: ForegroundPositive | ||
294 | } | ||||
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; | 295 | break; | ||
269 | case Information: | 296 | case Information: { | ||
270 | bgBaseColor.setRgb(61, 174, 233); // Window: ForegroundActive | 297 | if (settings && settings->contains(QStringLiteral("ForegroundActive"))) { | ||
298 | bgBaseColor = qColorFromSettings(settings, QStringLiteral("ForegroundActive")); | ||||
299 | } else { | ||||
300 | bgBaseColor = palette.highlight().color(); | ||||
301 | } | ||||
302 | // the alpha component is based on the perceived brightness (CIE-XYZ 1931) of the background colour | ||||
303 | // and its difference with receiving widget's background intensity: half the average of those values, | ||||
304 | // constrained by the background intensity itself. | ||||
305 | qreal bgIntensity = 0.299 * bgBaseColor.redF() + 0.586 * bgBaseColor.greenF() + 0.115 * bgBaseColor.blueF(); | ||||
306 | qreal windowIntensity = 0.299 * windowColor.redF() + 0.586 * windowColor.greenF() + 0.115 * windowColor.blueF(); | ||||
307 | bgBaseColorAlpha = 0.5 * qMax((bgIntensity + (1 - windowIntensity)), bgIntensity); | ||||
"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. | |||||
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… | |||||
271 | break; | 308 | break; | ||
309 | } | ||||
272 | case Warning: | 310 | case Warning: | ||
311 | if (settings && settings->contains(QStringLiteral("ForegroundNeutral"))) { | ||||
312 | bgBaseColor = qColorFromSettings(settings, QStringLiteral("ForegroundNeutral")); | ||||
313 | } else { | ||||
273 | bgBaseColor.setRgb(246, 116, 0); // Window: ForegroundNeutral | 314 | bgBaseColor.setRgb(246, 116, 0); // Window: ForegroundNeutral | ||
315 | } | ||||
274 | break; | 316 | break; | ||
275 | case Error: | 317 | case Error: | ||
318 | if (settings && settings->contains(QStringLiteral("ForegroundNegative"))) { | ||||
319 | bgBaseColor = qColorFromSettings(settings, QStringLiteral("ForegroundNegative")); | ||||
320 | } else { | ||||
276 | bgBaseColor.setRgb(218, 68, 83); // Window: ForegroundNegative | 321 | bgBaseColor.setRgb(218, 68, 83); // Window: ForegroundNegative | ||
322 | } | ||||
277 | break; | 323 | break; | ||
278 | } | 324 | } | ||
279 | const qreal bgBaseColorAlpha = 0.2; | 325 | if (settings) { | ||
326 | settings->endGroup(); | ||||
327 | delete settings; | ||||
328 | } | ||||
329 | | ||||
280 | bgBaseColor.setAlphaF(bgBaseColorAlpha); | 330 | bgBaseColor.setAlphaF(bgBaseColorAlpha); | ||
281 | 331 | | |||
282 | const QPalette palette = QGuiApplication::palette(); | | |||
ngraham: Same | |||||
283 | const QColor windowColor = palette.window().color(); | | |||
284 | const QColor textColor = palette.text().color(); | 332 | const QColor textColor = palette.text().color(); | ||
285 | const QColor border = bgBaseColor; | 333 | const QColor border = bgBaseColor; | ||
286 | 334 | | |||
287 | // Generate a final background color from overlaying bgBaseColor over windowColor | 335 | // Generate a final background color from overlaying bgBaseColor over windowColor | ||
288 | const int newRed = (bgBaseColor.red() * bgBaseColorAlpha) + (windowColor.red() * (1 - bgBaseColorAlpha)); | 336 | const int newRed = (bgBaseColor.red() * bgBaseColorAlpha) + (windowColor.red() * (1 - bgBaseColorAlpha)); | ||
289 | const int newGreen = (bgBaseColor.green() * bgBaseColorAlpha) + (windowColor.green() * (1 - bgBaseColorAlpha)); | 337 | const int newGreen = (bgBaseColor.green() * bgBaseColorAlpha) + (windowColor.green() * (1 - bgBaseColorAlpha)); | ||
290 | const int newBlue = (bgBaseColor.blue() * bgBaseColorAlpha) + (windowColor.blue() * (1 - bgBaseColorAlpha)); | 338 | const int newBlue = (bgBaseColor.blue() * bgBaseColorAlpha) + (windowColor.blue() * (1 - bgBaseColorAlpha)); | ||
291 | 339 | | |||
▲ Show 20 Lines • Show All 204 Lines • Show Last 20 Lines |
missing if (rgb.size() >= 3)