D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

ngraham added a comment. In D13777#286557 , @rjvbb wrote: > > That just doesn't look good, sorry. > > Again, that's an argument one should avoid. It's too subjective. I myself find it looks much better (I'm biased against everything Breeze,

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb added a comment. Seems my reply per email went AWOL: This is a work-in-progress ticket, but I can change the title because it is indeed not just about reverting a regression. > That just doesn't look good, sorry. Again, that's an argument one should avoid. It's too

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

ngraham requested changes to this revision. ngraham added a comment. This revision now requires changes to proceed. In D13777#286439 , @rjvbb wrote: > Have you guys considered using the 4 colours in question only for the message text and outer

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

ngraham added a reviewer: VDG. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D13777 To: rjvbb, ngraham, #frameworks, #vdg Cc: aacid, cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb added a comment. Since there's been mention about aligning Kirigami and re: the (IMHO) controversial approach of using foreground (text) colours for background purposes: Have you guys considered using the 4 colours in question only for the message text and outer frame, keeping

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

aacid added a comment. Please don't use kdeglobals, kdeglobals has to die, it's a "KDE 4" thing, there's no such thing as "KDE" anymore so using kdeglobals is a very bad idea. INLINE COMMENTS > kmessagewidget.cpp:272 > case Positive: > -bgBaseColor.setRgb(39, 174, 96); //

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb set the repository for this revision to R236 KWidgetsAddons. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D13777 To: rjvbb, ngraham, #frameworks Cc: cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb updated this revision to Diff 37000. rjvbb added a comment. KThemeSettings moved to its own header+implementation files, and extended so different groups can be read. I don't think it needs much else in terms of functionality or complexity, if anything. There's no point in exporting it

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

ngraham added a comment. Thanks for your continued work on this. I continue to believe that in order to move forward, it needs to be broken up into separate atomic commits: - One to add a color parsing function to `KWidgetsAddons` so other local clients can use it - Another to adopt

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb marked 8 inline comments as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D13777 To: rjvbb, ngraham, #frameworks Cc: cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb updated this revision to Diff 36943. rjvbb added a comment. Turns out it's trivial to check if and what custom theme was activated through the KColorSchemeManager. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13777?vs=36939=36943 REVISION DETAIL

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb set the repository for this revision to R236 KWidgetsAddons. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D13777 To: rjvbb, ngraham, #frameworks Cc: cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb added a comment. A few examples showing the subtle effect of brightness matching, comparing to the Breeze theme (the brightness reference): Oxygen: F5979829: breeze-vs-oxygen-vs-oxygen-matched.png Obsidian Coast F5979830:

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb set the repository for this revision to R236 KWidgetsAddons. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D13777 To: rjvbb, ngraham, #frameworks Cc: cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb updated this revision to Diff 36939. rjvbb added a comment. Using the theme's normal foreground colour for message text for more reliable readability with darker themes. Brightness matching to the fallback colour should never occur here, at least not when the palette text colour

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

cfeck added a comment. I don't understand. The brightness should be as close as possible to the brightness of the window background, that's why we only used a small blending factor. If your window text color is black, and your hightlight background is also black (with white highlight text),

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb updated this revision to Diff 36886. rjvbb added a comment. A new revision: - makes a start with the requested export of the QSettings functionality - drops the alpha tweak and introduces a new experiment: Since background colours are not obtained from theme colours designed

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb set the repository for this revision to R236 KWidgetsAddons. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D13777 To: rjvbb, ngraham, #frameworks Cc: cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

cfeck added a comment. I like the refactoring, much easier to read. I have to agree with Nathan that the special casing for the alpha is unneeded. For users having a black selection color, it wouldn't tint at all. We deliberately used a small blending value (0.2) to make sure the

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb set the repository for this revision to R236 KWidgetsAddons. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D13777 To: rjvbb, ngraham, #frameworks Cc: cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb updated this revision to Diff 36858. rjvbb added a comment. The need for an extra check in `qColorFromSettings()` meant I changed the API to put all tests in there and added a default return value argument. IOW, basically the same API as KConfig has. I've simplified the

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

cfeck added inline comments. INLINE COMMENTS > kmessagewidget.cpp:263 > +QStringList rgb = settings->value(key).toStringList(); > +return QColor(rgb.at(0).toInt(), rgb.at(1).toInt(), rgb.at(2).toInt()); > +} missing `if (rgb.size() >= 3)` > kmessagewidget.cpp:281 > +const QString

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb added inline comments. INLINE COMMENTS > ngraham wrote in kmessagewidget.cpp:311 > 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

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

ngraham added inline comments. INLINE COMMENTS > rjvbb wrote in kmessagewidget.cpp:311 > 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

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb marked 2 inline comments as done. rjvbb added inline comments. INLINE COMMENTS > ngraham wrote in kmessagewidget.cpp:311 > 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

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

ngraham added a comment. I just realized that we have another case in `KWidgetsAddons` that could benefit from your read-theme-colors-without-using-kconfig idea: D12756: [KDateTable] Use more appropriate and readable text colors for weekends and holidays

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

ngraham added inline comments. INLINE COMMENTS > ngraham wrote in kmessagewidget.cpp:275 > This commented-out line and its comments should be removed. Never mind, you were too fast for me. :) REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D13777 To: rjvbb,

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

ngraham added a comment. Thanks, I think this approach is more feasible! I have some comments below: INLINE COMMENTS > kmessagewidget.cpp:275 > +// for best results. > +// const QColor windowColor = KMessageWidget::palette().window().color(); > +const QColor windowColor =

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb updated this revision to Diff 36846. rjvbb added a comment. cleanup REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13777?vs=36845=36846 REVISION DETAIL https://phabricator.kde.org/D13777 AFFECTED FILES src/kmessagewidget.cpp To: rjvbb,

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb set the repository for this revision to R236 KWidgetsAddons. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D13777 To: rjvbb, ngraham, #frameworks Cc: cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb updated this revision to Diff 36845. rjvbb added a comment. Version using the suggested colour values from ~/.config/kdeglobals , if the file exists and has a Colors:Window group. In my case this gives informational and error messages almost the same background colour because I

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

ngraham added a comment. This patch isn't the right place for discussing changing the conceptual color scheme for the widgets. This patch also isn't the place for insulting the people who you're hoping will accept your changes. :) If you want to implement a private method to get

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb added a comment. > Another idea would have been to have the widget style alter the appearance like it already does for things like `KTitleWidget` Care to develop? > - Suspend output in a Konsole window that has a dark background color: the text is unreadable. By hitting

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

ngraham requested changes to this revision. ngraham added a comment. This revision now requires changes to proceed. I'm not sure that's the right approach. This new version has bugs with Breeze light: - Suspend output in a Konsole window that has a dark background color: the text is

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

broulik added a comment. -1 The upper two in second revision look quite bad. Another idea would have been to have the widget style alter the appearance like it already does for things like `KTitleWidget` REPOSITORY R236 KWidgetsAddons REVISION DETAIL

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb added a comment. 5.47.0 (stock) vs. 5.42.0 (stock), QtCurve (top) vs. Macintosh native (below): F5967082: KMessageWidget-styles-before,after.png with revision 2 of my patch: F5967086: Screen Shot 2018-06-28 at 16.04.28.png

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb marked 3 inline comments as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D13777 To: rjvbb, ngraham, #frameworks Cc: cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb set the repository for this revision to R236 KWidgetsAddons. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D13777 To: rjvbb, ngraham, #frameworks Cc: cfeck, kde-frameworks-devel, michaelh, ngraham, bruns

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb updated this revision to Diff 36841. rjvbb added a comment. This revision incorporates the feedback and implements the approach already evoked: - use the user's highlight colour for positive messages, with a 0.4 alpha value - use the user's tooltip background colour for

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

ngraham requested changes to this revision. ngraham added a comment. This revision now requires changes to proceed. KWidgetsAddons is a Tier 1 framework, so it can't depend on any other KDE Frameworks, including KConfig, which is where the current theme's full palette is stored. Because of

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

cfeck added a comment. here is WIP feedback: the comments explain the change, but they are not needed to explain the code. Remove them. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D13777 To: rjvbb, ngraham, #frameworks Cc: cfeck, kde-frameworks-devel,

D13777: KMessageWidget : revert to using highlight colour for Information style (WIP)

rjvbb created this revision. rjvbb added reviewers: ngraham, Frameworks. Restricted Application added a project: Frameworks. rjvbb requested review of this revision. REVISION SUMMARY A recent commit (00bce130d35e9dc398709e690a05f8dde70f52b3