Fix: apply correctly the text colors of the chosen scheme
ClosedPublic

Authored by nibags on Mar 19 2019, 7:04 AM.

Details

Summary

BUG: 398758

Since the implementation of KSyntaxHighlighting in KF5.50, KTextEditor always uses the "Normal" scheme for text colors.

Previously, the colors were set according to KSyntaxHighlighting::Repository::LightTheme, which corresponds to the "Normal" scheme. Now use KateRendererConfig::global() to get the name of the current selected scheme.

Before:

After:

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nibags created this revision.Mar 19 2019, 7:04 AM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptMar 19 2019, 7:04 AM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
nibags requested review of this revision.Mar 19 2019, 7:04 AM
nibags edited the summary of this revision. (Show Details)Mar 19 2019, 7:11 AM
nibags added reviewers: KTextEditor, Kate.
nibags updated this revision to Diff 54404.Mar 20 2019, 10:53 AM
  • Fix theme names

Convert theme names from KTextEditor => KSyntaxHighlighting and avoid using invalid KSyntaxHighlighting::Theme objects.

The KDE and vim (dark) themes don't exist in KSyntaxHighlighting, in such cases KSyntaxHighlighting::Repository::LightTheme is used (however, the colors of the original theme are retained).

nibags updated this revision to Diff 54614.Mar 23 2019, 1:54 PM
  • Add some comments

I think such a workaround is ok, but the real issue here is that KTextEditor does not fully use the KSyntaxHighlighting Theme colors. Instead, it still has its own configuration, such that hacks like this are introduced to somehow make it work. But it's just a matter of time until it breaks again.

Any other comments?

I don't like that one sets the global renderer config.

Where attributesForDefinition is called one knows our schema name. One could just pass it to that routine and do the hack only there.

cullmann requested changes to this revision.Sun, Mar 24, 1:07 PM
This revision now requires changes to proceed.Sun, Mar 24, 1:07 PM
nibags updated this revision to Diff 54702.Sun, Mar 24, 6:06 PM
  • Pass schema name as parameter

I've also done debug and everything works as it should.
Any problem or detail to change, don't hesitate to say

nibags edited the summary of this revision. (Show Details)Sun, Mar 24, 6:09 PM
cullmann accepted this revision.Sun, Mar 24, 6:37 PM

Ok with that.
In the long run we should use only the syntaxhighlighting themes and provide UX for that.
Volunteers?

This revision is now accepted and ready to land.Sun, Mar 24, 6:37 PM
mwolff requested changes to this revision.Sun, Mar 24, 9:12 PM
mwolff added a subscriber: mwolff.

one minor nit, otherwise looks like a good improvement

src/syntax/katehighlight.cpp
78

return QStringLiteral, otherwise you allocate on every function call (also below)

This revision now requires changes to proceed.Sun, Mar 24, 9:12 PM
nibags updated this revision to Diff 54775.Mon, Mar 25, 2:26 PM
  • Use QStringLiteral
cullmann accepted this revision.Thu, Mar 28, 8:23 AM

I am ok with this ;=)

nibags updated this revision to Diff 55004.Thu, Mar 28, 10:15 PM
nibags marked an inline comment as done.
  • Fix attributes in "KDE" and "Vim (dark)" themes

I found a small bug in this patch. In the "KDE" and "Vim (dark)" schemas, hard colors and text backgrounds, defined in XML files, don't work. I have updated the diff: now, for these cases, an empty theme will be used and only the attributes defined in the XML files will be applied, such as bold="1" or color="#CCCC".

nibags added a comment.Tue, Apr 2, 6:32 AM

I have tested this a lot and it works well so far. I don't know if you have any disagreement with the last update (for example, using setTheme() with an empty theme, although I haven't seen this cause any problems)...

cullmann accepted this revision.Tue, Apr 2, 9:13 AM

I think this is good enough as it is.
I would prefer that we in the future just switch over to use the syntax-highlighting themes, but that is a lot of work :)

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Apr 3, 7:22 AM
This revision was automatically updated to reflect the committed changes.
nibags added a comment.Wed, Apr 3, 7:40 AM

Sorry, I committed this diff, but it hasn't been fully accepted. Although I made the changes that mwolff requested. If there is a problem, I can undo the commit or update it....