Text previews are unreadable (in fact look empty) with dark colour schemes because of the hard-coded background color.
BUG: 406404
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
| thumbnail/textcreator.cpp | ||
|---|---|---|
| 140 | Can't you use QPalette instead of KColorScheme? The latter parses config files on disk and is somewhat heavy | |
| thumbnail/textcreator.cpp | ||
|---|---|---|
| 7 | Too many lines removed here, it should not compile anymore. | |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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?
Comment Actions
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.
Comment Actions
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).
Comment Actions
I would welcome the same palette in the file preview as the one I see in ktexteditor when open the file
Comment Actions
@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?
Comment Actions
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)
Comment Actions
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.
Comment Actions
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.
Comment Actions
I believe the issue currently is that for text files or files without associated definition we end up with an invalid definition that prevents SyntaxHighlighter::setDefinition to call rehighlight().
QTextDocument uses then whatever it get from the theme I guess.
Kate has an explicit special case in KateHighlighting::KateHighlighting to insert a default Format.
I think either KSyntaxHighlighting should have a dummy default definition based on currrent theme or we should here create a dummy default definition when m_highlightingRepository.definitionForFileName(path) return an invalid definition.
What do you think ?
Comment Actions
I have opened https://phabricator.kde.org/D25323 to fix the missing highlighting in kio-extras.