Differential D14274

Modelines: add byte-order-mark & small fixes
ClosedPublic
Actions

Authored by nibags on Jul 22 2018, 12:57 PM.

Details

Reviewers
turbov
dhaumann
cullmann
vkrause
Group Reviewers
Kate
Commits
R216:e1c3d16a35dd: Modelines: add byte-order-mark & small fixes
R216:bffb66cb3ed8: Modelines: add byte-order-mark & small fixes
Summary

Changes:

  • Add missing keyword: byte-order-mark (there is only "byte-order-marker" and "bom"). Obtained from: https://docs.kde.org/stable5/en/applications/katepart/config-variables.html
  • Remove keywords delimiters "*" and "+", because in the "RemoveSpacesOptions" list, the keywords "*" and "+" are not highlighted.
  • Fix ";" in the "RemoveSpaces" context ("remove-trailing-spaces" variable). The character ";" it is not highlighted as in the rest of the variables, since when writing a value, the context ends.

  • For a strange reason, the use of <IncludeRules context="##Modelines"/>, in others files, causes the line continuation character ("\") to be highlighted as dsNormal. This is solved by removing the LineContinue rule in the "Normal" context in Modelines. I consider the LineContinue & DetectSpaces rules to be unnecessary in the "Normal" context, so I've eliminated them.

I was going to add a test file, but there are errors when generating the html and ref files, even with the unmodified modelines.xml file :S

Diff Detail

Repository
R216 Syntax Highlighting
Branch
fix-modelines
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1064
Build 1077: arc lint + arc unit
nibags created this revision.Jul 22 2018, 12:57 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptJul 22 2018, 12:57 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
nibags requested review of this revision.Jul 22 2018, 12:57 PM
nibags edited the summary of this revision. (Show Details)Jul 22 2018, 1:13 PM
nibags added reviewers: turbov, dhaumann, cullmann, vkrause, Kate.
nibags updated this revision to Diff 38477.Jul 26 2018, 7:29 AM
  • Delete update references
cullmann requested changes to this revision.Aug 13 2018, 6:13 AM

<DetectSpaces /> is only a performance optimization, as it just skips over whitespaces before any of the costly rules are tried to match. I think that should just be added again.
Otherwise, I have no issues with this changes.
What was exactly the problem with the unit test? Having one would be nice.

This revision now requires changes to proceed.Aug 13 2018, 6:13 AM
dhaumann added a comment.Aug 13 2018, 6:32 AM

Sidenote: Looking into the code in KTextEditor, there is:

 } else if (var == QLatin1String("bom") || var == QLatin1String("byte-order-mark") || var == QLatin1String("byte-order-marker")) {
    if (checkBoolValue(val, &state)) {
        m_config->setBom(state);
    }
}

So byte-order-mark is also supported, the longer version byte-order-marker is just there for backwards compatibility.

nibags added a comment.Aug 13 2018, 7:04 AM

So I keep the DetectSpaces rule and only delete the LineContinue.

By having a file with Modelines as a test, randomly, different .html and .ref files are generated each time I run "testhighlighter_test" and "htmlhighlighter_test". That is, sometimes the .html and .ref files show the Modelines highlighted and sometimes not. Also, the word kate: should be highlighted with "dsAnnotation" (which is pink in the Normal scheme). I attach a screenshot of two generated .html files.

The test file:

modelines.sh232 BDownload

nibags added a comment.Aug 13 2018, 7:08 AM

I will update the diff including the tests.

nibags updated this revision to Diff 39551.Aug 13 2018, 7:31 AM
  • Add test & add DetectSpace.
nibags updated this revision to Diff 39552.Aug 13 2018, 7:42 AM
  • Corrects references erroneously included in previous update
cullmann accepted this revision.Aug 13 2018, 7:45 AM

OK, thanks.

This revision is now accepted and ready to land.Aug 13 2018, 7:45 AM
Closed by commit R216:bffb66cb3ed8: Modelines: add byte-order-mark & small fixes (authored by nibags, committed by cullmann). · Explain WhyAug 14 2018, 2:55 PM
This revision was automatically updated to reflect the committed changes.
cullmann reopened this revision.Aug 14 2018, 3:16 PM

I needed to revert this again, as we get tests failures:

https://build.kde.org/job/Frameworks%20syntax-highlighting%20kf5-qt5%20SUSEQt5.9/97/testReport/junit/(root)/TestSuite/testhighlighter_test/

-<Comment># </Comment><Keyword>kate:</Keyword><Comment> </Comment><Variable>syntax</Variable><String> Bash </String><Variable>;</Variable><Comment> </Comment><Variable>scheme</Variable><String> Normal</String><Variable>;</Variable><br/>
-<Comment># </Comment><Keyword>kate:</Keyword><Comment> </Comment><Variable>font-size</Variable><Comment> </Comment><Number>14</Number><Variable>;</Variable><Comment> </Comment><Variable>background-color</Variable><String> #FAFAFA </String><Variable>;</Variable><br/>
+<Comment># kate: syntax Bash ; scheme Normal;</Comment><br/>
+<Comment># kate: font-size 14; background-color #FAFAFA ;</Comment><br/>

This revision is now accepted and ready to land.Aug 14 2018, 3:16 PM
cullmann requested changes to this revision.Aug 14 2018, 3:17 PM
This revision now requires changes to proceed.Aug 14 2018, 3:17 PM
nibags updated this revision to Diff 39789.Aug 15 2018, 11:25 AM
  • Fix test (use Modelines in Python instead of Bash)
cullmann added a comment.Aug 15 2018, 6:12 PM

Will try that out again.

cullmann accepted this revision.Aug 15 2018, 6:22 PM

Works, will integrate, thanks for the patch!

This revision is now accepted and ready to land.Aug 15 2018, 6:22 PM
cullmann closed this revision.Aug 15 2018, 6:24 PM

Git commit e1c3d16a35ddae90de6e457a31f54d899abb84c8 by Christoph Cullmann, on behalf of Nibaldo González.
Committed on 15/08/2018 at 18:22.
Pushed by cullmann into branch 'master'.

Modelines: add byte-order-mark & small fixes

Summary:
Changes:

  • Add missing keyword: byte-order-mark (there is only "byte-order-marker" and "bom"). Obtained from: https://docs.kde.org/stable5/en/applications/katepart/config-variables.html
  • Remove keywords delimiters "*" and "+", because in the "RemoveSpacesOptions" list, the keywords "*" and "+" are not highlighted.
  • Fix ";" in the "RemoveSpaces" context ("remove-trailing-spaces" variable). The character ";" it is not highlighted as in the rest of the variables, since when writing a value, the context ends.

  • For a strange reason, the use of <IncludeRules context="##Modelines"/>, in others files, causes the line continuation character ("\") to be highlighted as dsNormal. This is solved by removing the LineContinue rule in the "Normal" context in Modelines. I consider the LineContinue & DetectSpaces rules to be unnecessary in the "Normal" context, so I've eliminated them.

I was going to add a test file, but there are errors when generating the html and ref files, even with the unmodified modelines.xml file :S

Reviewers: turbov, dhaumann, cullmann, vkrause, Kate

Reviewed By: cullmann, Kate

Subscribers: kwrite-devel, kde-frameworks-devel

Tags: Kate, Frameworks

Differential Revision: https://phabricator.kde.org/D14274

A +10 -0 autotests/folding/modelines.py.fold
A +17 -0 autotests/html/modelines.py.html
A +10 -0 autotests/input/modelines.py
A +10 -0 autotests/reference/modelines.py.ref
M +7 -4 data/syntax/modelines.xml

https://commits.kde.org/syntax-highlighting/e1c3d16a35ddae90de6e457a31f54d899abb84c8

Revision Contents
Changeset List

Diff 38201

View Options

autotests/reference/highlight.css.ref

Loading...
View Options

autotests/reference/review128925-1.css.ref

Loading...
View Options

autotests/reference/review128925-2.css.ref

Loading...
View Options

data/syntax/modelines.xml

Loading...