Differential D4637

KColorScheme: read application's KDE_COLOR_SCHEME_PATH property
ClosedPublic
Actions

Authored by flherne on Feb 16 2017, 10:37 PM.

Details

Reviewers
graesslin
Group Reviewers
Frameworks
Commits
R265:a3355b22954b: KColorScheme: default to application scheme if set by KColorSchemeManager
Summary

[RFC]

KColorSchemeManager::activateScheme() sets a custom path for the application's color scheme, with

qApp->setProperty("KDE_COLOR_SCHEME_PATH", index.data(Qt::UserRole));

Currently, the KColorScheme() and KStatefulBrush() constructors will ignore this and use only the system color scheme, unless an application-specifiic config is explicitly loaded and passed in by the caller.

This is problematic, because all callers I've seen assume that the default is to match the application scheme (usually this is equivalent, because few applications use KColorSchemeManager).

For example, when the application of a KTextEditor widget or KonsolePart has an opposite color scheme to the system, the Find bars are unreadable: https://bugs.kde.org/373764

This patch makes KColorScheme() match the application scheme by default when this differs from the system scheme, which seems preferable to adding the same code in hundreds of callers.

BUG: 373764

Test Plan

It works...

Various UI elements in KDevelop (and loaded kparts) now use the application scheme and are readable.

There doesn't seem to be a unit-test system.

Diff Detail

Repository
R265 KConfigWidgets
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
flherne updated this revision to Diff 11417.Feb 16 2017, 10:37 PM
flherne retitled this revision from to KColorScheme: read application's KDE_COLOR_SCHEME_PATH property.
flherne updated this object.
flherne edited the test plan for this revision. (Show Details)
flherne added a reviewer: Frameworks.
flherne set the repository for this revision to R265 KConfigWidgets.
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 16 2017, 10:37 PM
graesslin added a subscriber: graesslin.Feb 17 2017, 6:16 AM

This is a sensible idea, though I wonder whether we should put it into something more concrete than a QProperty on the qApp.

src/kcolorscheme.h
33–37

in case the kcolorscheme.h gets installed, I suggest to not inline the method.

flherne updated this revision to Diff 11438.Feb 17 2017, 8:49 AM

Moved defaultConfig() out of the header in case someone includes it.

flherne updated this revision to Diff 11439.Feb 17 2017, 9:06 AM

Tweak a comment.

flherne added a comment.Feb 17 2017, 9:11 AM
In D4637#87022, @graesslin wrote:

This is a sensible idea, though I wonder whether we should put it into something more concrete than a QProperty on the qApp.

That's not new, the property's been set here since at least the kdelibs split.

graesslin added a comment.Feb 17 2017, 1:45 PM

That's not new, the property's been set here since at least the kdelibs split.

I know, I was the one adding it :-) That's also why I think we could do that better than a property: it was intended as a way to communicate with the QStyle - the aim of the property is to inform kwin which color scheme the window uses and to adjust the decoration.

So if we use this in more places it might be better to not go through a property, but through a proper manager instance. But that's totally orthogonal to your change, of course.

graesslin added inline comments.Feb 17 2017, 1:48 PM
src/kcolorscheme.h
316–318

I wouldn't mention the property name - that can be considered an implementation detail. Maybe instead something like "If a color scheme got installed with a KColorSchemeManager that one is used".

flherne updated this revision to Diff 11471.EditedFeb 17 2017, 9:19 PM
flherne edited the test plan for this revision. (Show Details)

Update the comments / docstrings

flherne marked an inline comment as done.Feb 17 2017, 10:41 PM
graesslin accepted this revision.Feb 19 2017, 2:29 PM
This revision is now accepted and ready to land.Feb 19 2017, 2:29 PM
Closed by commit R265:a3355b22954b: KColorScheme: default to application scheme if set by KColorSchemeManager (authored by flherne). · Explain WhyFeb 19 2017, 8:46 PM
This revision was automatically updated to reflect the committed changes.
kfunk added a subscriber: kfunk.Feb 20 2017, 7:54 AM

Thanks for the investigation, Francis! \o/

Revision Contents
Changeset List

PathPackages
Msrc/kcolorscheme.h (30 lines)
Msrc/kcolorscheme.cpp (13 lines)

Diff 11508

View Options

src/kcolorscheme.h

Loading...
View Options

src/kcolorscheme.cpp

Loading...