SyntaxHighlighter: Fix foreground color for text without special highlighting
ClosedPublic

Authored by dhaumann on May 19 2019, 6:22 PM.

Details

Summary

QTextDocument seems to use the widget palette (or application
palette) when drawing text. That is, by default the palette's
foreground color is used when no special QTextCharFormat is
specified.

This patch changes applyFormat() such that the foreground color
is always set to avoid the fallback to the QPalette's foreground
color.

BUG: 406821
BUG: 406816
FIXED-IN: 5.59

Test Plan

make && make test + manual testing with the codeeditor example

Diff Detail

Repository
R216 Syntax Highlighting
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dhaumann created this revision.May 19 2019, 6:22 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptMay 19 2019, 6:22 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
dhaumann requested review of this revision.May 19 2019, 6:22 PM

The text thumbnailer was added in: D19432.

I think one reason for the early out was that actually even the "isXXX" checks are very expensive, at least if I remember correctly they did show up a lot in my profiling in the past.
Perhaps one should profile this once more (and if still visible) at least skip all the isXXX checks for the isDefault... case.
Otherwise I have no issues with this.

Well, this change only affects the SyntaxHighlighter, which KTextEditor does not use ;)

For completeness: https://code.woboq.org/qt5/qtbase/src/gui/text/qtextdocument.cpp.html#673
Here, we can see that QTextDocument::drawContents() internally uses a QAbstractTextDocumentLayout::PaintContext ctx, which itself provides a default-constructed QPalette. A defaut-constructed QPalette equals the application palette. So the only other way for the text thumbnailer is to temporarily set the QApplication palette.

I can confirm that this fixes the thumbnail bug for me: when I change the color theme, get rid of current thumbnail process (killall thumbnail.so) and trigger a reload of text thumbnails in Dolphin, I now always get consistently same colors with enough contrast used.

@cullmann I don't see how we can optimize unneed hasXXX calls away: Format::isDefaultTextStyle(const Theme &theme) internally uses the same 'hasXXX' calls, i.e. if we use both, we do it twice. Right now I don't see a trivial way to improve the patch.

:=) You are right, without that, it might be even faster as we only do it once.

cullmann accepted this revision.May 22 2019, 2:28 PM

Given nobody proposes a working alternative, lets go with this.

This revision is now accepted and ready to land.May 22 2019, 2:28 PM
This revision was automatically updated to reflect the committed changes.