Use appropriate background color for text previews
Needs RevisionPublic

Authored by eshalygin on Apr 23 2019, 9:42 AM.

Details

Reviewers
kossebau
cfeck
Summary

Text previews are unreadable (in fact look empty) with dark colour schemes because of the hard-coded background color.

BUG: 406404

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
eshalygin created this revision.Apr 23 2019, 9:42 AM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptApr 23 2019, 9:42 AM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
eshalygin requested review of this revision.Apr 23 2019, 9:42 AM
broulik added inline comments.
thumbnail/textcreator.cpp
140

Can't you use QPalette instead of KColorScheme? The latter parses config files on disk and is somewhat heavy

eshalygin updated this revision to Diff 56806.Apr 23 2019, 9:59 AM

Replace KColorScheme with QPalette

meven added a subscriber: meven.Apr 23 2019, 10:05 AM
meven added inline comments.
thumbnail/textcreator.cpp
7

Too many lines removed here, it should not compile anymore.
Just a small editing mistake I guess.

eshalygin marked an inline comment as done.Apr 23 2019, 10:10 AM
kossebau added a subscriber: cfeck.EditedApr 23 2019, 1:08 PM

Thanks for the patch. Hm, somehow I remember from my testing for D19432 that highlighting color did not take active UI color theme into account, has that changed meanwhile?

In any case, IMHO there should be first a decision whether thumbnails for plain text documents should be done taking the UI color theme into account, or should be similar to pdf, doc & Co, where usually a print out on paper is assumed for the look (at least by the native formats). Personally I would favour the latter, and the one doing the hard-coded background color (@cfeck) seems at one point done to have as well.

ngraham edited the summary of this revision. (Show Details)Apr 23 2019, 1:13 PM

BTW, just a note while testing: while the thumbnails curently are not cached but always rerendered on demand (due to flag in metadata), there currently is no update mechanism to update the thumbnails already shown e.g. in Dolphin when the UI color theme changes.

kossebau requested changes to this revision.Apr 23 2019, 1:42 PM
kossebau added a subscriber: vkrause.

For one, I cannot reproduce the bug. With all the dark UI color themes I tested, I always had proper contrasted dark text colors in the plain text thumbnail.

Then, whatever people prefer how the thing should be rendered, the current patch proposed though does not go well along the current codebase where it asks KSyntaxHighlighting for a theme:

const auto highlightingTheme = m_highlightingRepository.defaultTheme(KSyntaxHighlighting::Repository::LightTheme);

"LightTheme: Theme with a light background color. " (see API dox

@vkrause What would you recommend us to do here, in case people want previews matching the current UI theme? For the other case, how could it happen that "LightTheme" seems to give some people bright highlight colors for the people affected?

This revision now requires changes to proceed.Apr 23 2019, 1:42 PM
kossebau added a comment.EditedApr 23 2019, 1:58 PM

So, switching also the Plasma Theme to "Breeze Dark", I started to be able to reproduce: seems that the default text color is influenced by that. Which seems a side-effect not to be expected. And this patch here just a work-around for the symptom. We ask KSyntaxHighlighting::Repository for a light theme, we do expect one.
IMHO this seems rather a bug in KSyntaxHighlighting and should be fixed there.

cfeck requested changes to this revision.Apr 23 2019, 2:02 PM

Before we can support both dark and light themes we need to investigate the KSyntaxHighlighting issue found by Friedrich.

Later we could request a light or dark theme depending on the lightness of the QPalette entry, but I would suggest to use View instead of Window, because window backgrounds are gray with some themes, while view backgrounds usually have more contrast (either darker or brighter than gray).

I would welcome the same palette in the file preview as the one I see in ktexteditor when open the file

So, switching also the Plasma Theme to "Breeze Dark", I started to be able to reproduce: seems that the default text color is influenced by that. Which seems a side-effect not to be expected. And this patch here just a work-around for the symptom. We ask KSyntaxHighlighting::Repository for a light theme, we do expect one.
IMHO this seems rather a bug in KSyntaxHighlighting and should be fixed there.

@dhaumann @cullmann ^ this sounds like we are making the assumption that the current system color theme from the palette matches the syntax highlighting theme, ie. the palette provided text color is compatible with the selected theme. That's probably true in the common editor case, but it seems to be an issue here. Should the syntax highlighting themes always have a hardcoded text color? Doesn't seem entirely clean either, as following the system color makes sense for different color schemes with the same "lightness"/"darkness". Or should we try to detect insufficient contrast between foreground and background, and use an alternate color in that case?

kossebau added a comment.EditedApr 23 2019, 3:07 PM

I am even more confused that switching back to "Breeze" from "Breeze Dark", I can still reproduce things when just switching to dark color themes only. So possibly that is enough, and I was before potentially fooled by some running processes, despite making sure no thumbnailer kio-slave was idling, or by some improperly propagated UI color palette changes).
So possibly indeed just the UI color palette effective here, Plasma unrelated.

Re: Kate: seems from its text document UI schemas "Normal", "Printing", "KDE", "Breeze Dark", "Solarized (light)", Solarized (dark", "Vim (dark)". the "KDE" one is the one which adapts to current workspace UI color theme, while the others all have a completely hard-coded palette?

@vkrause Given the API dox claiming that enum KSyntaxHighlighting::Repository::DefaultTheme lists "Built-in default theme types", to me it comes a bit of a surprise that the theme actually adapts to the UI color theme, as this is not documented. E.g. if one uses this for printing on paper, getting different results depending on current UI color theme would result in a bit randomness.

Perhaps there should be additional option(s) for default theme(s) which adapt to the UI theme. Though this also needs support for adapting in case the user changes the theme (e.g. think of discussion about day & night modes using different color themes).

So: IMHO the default themes should be hard-coded. And support for highlighting themes adapting to the currently active UI color palette should be officially added and documented.
(Update: filed https://bugs.kde.org/show_bug.cgi?id=406816 Or potentially just a bug in KSyntaxHighlighting where it would not apply the "Normal" textstyle? https://bugs.kde.org/show_bug.cgi?id=406821)

I actually think the theme should contain all colors hard-coded and KTextEditor should properly use that instead of currently the mix of hardcoded/defaults.
The only automatism should be (in my eyes) to switch between a light/dark variant automatic depending on the base color of the current window.
But that is only my opinion.
I think the "we adapt to the color scheme" stuff is flawed.

meven added a comment.Jun 29 2019, 9:41 AM

For context :

Given D21295, can we move forward here ?

For context :

Given D21295, can we move forward here ?

To give context to this comment: seems the bug symptom made its reappearance for other reasons yet again, I opened https://bugs.kde.org/show_bug.cgi?id=409380 to track & handle this.