D19876: Fix: apply correctly the text colors of the chosen scheme
nibags added a comment. 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 REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19876 To: nibags, #ktexteditor, #kate, cullmann, mwolff Cc: mwolff, cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, sars
D19876: Fix: apply correctly the text colors of the chosen scheme
This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit R39:c922bc8352ac: Fix: apply correctly the text colors of the chosen scheme (authored by nibags). REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19876?vs=55004=55328 REVISION DETAIL https://phabricator.kde.org/D19876 AFFECTED FILES src/syntax/katehighlight.cpp src/syntax/katehighlight.h To: nibags, #ktexteditor, #kate, cullmann, mwolff Cc: mwolff, cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, sars
D19876: Fix: apply correctly the text colors of the chosen scheme
cullmann accepted this revision. cullmann added a comment. 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 :) REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19876 To: nibags, #ktexteditor, #kate, cullmann, mwolff Cc: mwolff, cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, sars
D19876: Fix: apply correctly the text colors of the chosen scheme
nibags added a comment. 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)... REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19876 To: nibags, #ktexteditor, #kate, cullmann, mwolff Cc: mwolff, cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, sars
D19876: Fix: apply correctly the text colors of the chosen scheme
nibags updated this revision to Diff 55004. nibags marked an inline comment as done. nibags added a comment. - 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="#"`. REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19876?vs=54775=55004 BRANCH fix-schemas REVISION DETAIL https://phabricator.kde.org/D19876 AFFECTED FILES src/syntax/katehighlight.cpp src/syntax/katehighlight.h To: nibags, #ktexteditor, #kate, cullmann, mwolff Cc: mwolff, cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, sars
D19876: Fix: apply correctly the text colors of the chosen scheme
cullmann accepted this revision. cullmann added a comment. I am ok with this ;=) REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19876 To: nibags, #ktexteditor, #kate, cullmann, mwolff Cc: mwolff, cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, sars
D19876: Fix: apply correctly the text colors of the chosen scheme
nibags updated this revision to Diff 54775. nibags added a comment. - Use QStringLiteral REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19876?vs=54702=54775 BRANCH fix-schemas REVISION DETAIL https://phabricator.kde.org/D19876 AFFECTED FILES src/syntax/katehighlight.cpp src/syntax/katehighlight.h To: nibags, #ktexteditor, #kate, cullmann, mwolff Cc: mwolff, cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, sars
D19876: Fix: apply correctly the text colors of the chosen scheme
mwolff requested changes to this revision.
mwolff added a comment.
This revision now requires changes to proceed.
one minor nit, otherwise looks like a good improvement
INLINE COMMENTS
> katehighlight.cpp:78
> +if (schema == QLatin1String("Normal")) {
> +return QLatin1String("Default");
> +} else if (schema == QLatin1String("Solarized (light)")) {
return QStringLiteral, otherwise you allocate on every function call (also
below)
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D19876
To: nibags, #ktexteditor, #kate, cullmann, mwolff
Cc: mwolff, cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, gennad,
domson, michaelh, ngraham, bruns, demsking, sars
D19876: Fix: apply correctly the text colors of the chosen scheme
cullmann accepted this revision. cullmann added a comment. This revision is now accepted and ready to land. Ok with that. In the long run we should use only the syntaxhighlighting themes and provide UX for that. Volunteers? REPOSITORY R39 KTextEditor BRANCH fix-schemas REVISION DETAIL https://phabricator.kde.org/D19876 To: nibags, #ktexteditor, #kate, cullmann Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, sars
D19876: Fix: apply correctly the text colors of the chosen scheme
nibags edited the summary of this revision. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19876 To: nibags, #ktexteditor, #kate, cullmann Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, sars
D19876: Fix: apply correctly the text colors of the chosen scheme
nibags updated this revision to Diff 54702. nibags added a comment. - 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 REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19876?vs=54614=54702 BRANCH fix-schemas REVISION DETAIL https://phabricator.kde.org/D19876 AFFECTED FILES src/syntax/katehighlight.cpp src/syntax/katehighlight.h To: nibags, #ktexteditor, #kate, cullmann Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, sars
D19876: Fix: apply correctly the text colors of the chosen scheme
cullmann requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19876 To: nibags, #ktexteditor, #kate, cullmann Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, sars
D19876: Fix: apply correctly the text colors of the chosen scheme
cullmann added a comment. 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. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19876 To: nibags, #ktexteditor, #kate Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, sars
D19876: Fix: apply correctly the text colors of the chosen scheme
dhaumann added a comment. 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? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19876 To: nibags, #ktexteditor, #kate Cc: dhaumann, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, cullmann, sars
D19876: Fix: apply correctly the text colors of the chosen scheme
nibags updated this revision to Diff 54614. nibags added a comment. - Add some comments REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19876?vs=54404=54614 BRANCH fix-schemas REVISION DETAIL https://phabricator.kde.org/D19876 AFFECTED FILES src/schema/kateschema.cpp src/syntax/katehighlight.cpp To: nibags, #ktexteditor, #kate Cc: kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann
D19876: Fix: apply correctly the text colors of the chosen scheme
nibags updated this revision to Diff 54404. nibags added a comment. - 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). REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19876?vs=54295=54404 BRANCH fix-schemas REVISION DETAIL https://phabricator.kde.org/D19876 AFFECTED FILES src/schema/kateschema.cpp src/syntax/katehighlight.cpp To: nibags, #ktexteditor, #kate Cc: kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann
D19876: Fix: apply correctly the text colors of the chosen scheme
nibags edited the summary of this revision. nibags added reviewers: KTextEditor, Kate. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19876 To: nibags, #ktexteditor, #kate Cc: kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann
D19876: Fix: apply correctly the text colors of the chosen scheme
nibags created this revision. Herald added projects: Kate, Frameworks. Herald added subscribers: kde-frameworks-devel, kwrite-devel. nibags requested review of this revision. REVISION 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. REPOSITORY R39 KTextEditor BRANCH fix-schemas REVISION DETAIL https://phabricator.kde.org/D19876 AFFECTED FILES src/syntax/katehighlight.cpp To: nibags Cc: kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann
