Add AsciiDoc support
ClosedPublic

Authored by andreasgr on Jan 23 2019, 5:02 PM.

Details

Summary

AsciiDoc syntax highlighting aiming to behave like Asciidoctor and
use colours similar to its default style sheet.

Diff Detail

Repository
R216 Syntax Highlighting
Lint
Lint Skipped
Unit
Unit Tests Skipped
andreasgr created this revision.Jan 23 2019, 5:02 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptJan 23 2019, 5:02 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
andreasgr requested review of this revision.Jan 23 2019, 5:02 PM
andreasgr updated this revision to Diff 50263.Jan 25 2019, 3:11 PM
andreasgr removed projects: Frameworks, Kate.
  • Add missing allowed leading characters for links.
  • Add escaping of attribute usage.
  • Avoid highlighting of spaces inside table.
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptJan 25 2019, 3:11 PM
nibags added a subscriber: nibags.Jan 26 2019, 3:44 AM

Hi!!
Please, also add a test file in: "autotests/input/". To generate the files in "folding", "html" and "reference" use make test and autotests/update-reference-data.sh.

I am not a reviewer, but I want to add some recommendations in relation to the itemData's:

  • On lines 467, 476 and 485 you use harded colors for the fonts. Ideally, don't use them, but use the default styles, since a better visualization is achieved in the different schemes: https://kate-editor.org/2014/03/07/kate-part-kf5-new-default-styles-for-better-color-schemes/
  • On lines 469 & 471 you use background color, and on lines 466-467, 473-474, 479-481 and 490 you use background color, but not font color. Try not to use background color if it's not necessary, to get a better visualization in the different schemes. However, if you need background color, it also specifies the font color, to avoid readability problems in some color schemes (for example, the background color &monospaced_bg; with dsNormal is not readable in dark schemes).

Hi!!
Please, also add a test file in: "autotests/input/". To generate the files in "folding", "html" and "reference" use make test and autotests/update-reference-data.sh.

I am not a reviewer, but I want to add some recommendations in relation to the itemData's:

  • On lines 467, 476 and 485 you use harded colors for the fonts. Ideally, don't use them, but use the default styles, since a better visualization is achieved in the different schemes: https://kate-editor.org/2014/03/07/kate-part-kf5-new-default-styles-for-better-color-schemes/
  • On lines 469 & 471 you use background color, and on lines 466-467, 473-474, 479-481 and 490 you use background color, but not font color. Try not to use background color if it's not necessary, to get a better visualization in the different schemes. However, if you need background color, it also specifies the font color, to avoid readability problems in some color schemes (for example, the background color &monospaced_bg; with dsNormal is not readable in dark schemes).

Hi @nibags,

many thanks for your feedback.

I will see what I can do about the test file.

Regarding colours, this was mostly done to make the highlighting look similar to the default output created by Asciidoctor.
Your point about different schemes is a very valid one, though. I need to reconsider the style choices.

Cheers

With respect to colors: it's more important to be consistent with other highlightings in kate that with AsciiDoctor. Indeed, it would be nice if you can avoid any hard coded color.

andreasgr updated this revision to Diff 50382.Jan 27 2019, 2:59 PM

Drop custom colors and backgrounds.

Running the katesyntaxhighlighting indexer tells me:

katehighlightingindexer::KeywordChecker::check: "syntax-highlighting/data/syntax/asciidoc.xml" Unused keyword lists: "admonition"

Could you fix this? Usually, keyword completion should be context dependent anyways (currently a bug), so soon your unused list will indeed not have any effect. Besides: If you want to support specials like TODO, NOTE, ..., then please use IncludeRules and ##Alert (look into other files how it's done).

Also, could you please change all itemData names to upper case naming? Eg. "replacement" -> "Replacement", or "section title" -> Section Title", etc? All other highlighting files use this naming scheme, and it will be visible in the UI later when configuring the colors in Kate.

And what's still missing is a test file, also best licensed under MIT, that we can use for regression testing.

Running the katesyntaxhighlighting indexer tells me:

katehighlightingindexer::KeywordChecker::check: "syntax-highlighting/data/syntax/asciidoc.xml" Unused keyword lists: "admonition"

Could you fix this? Usually, keyword completion should be context dependent anyways (currently a bug), so soon your unused list will indeed not have any effect.

OK, I will remove it.
I just added it as this supports word completion.

Besides: If you want to support specials like TODO, NOTE, ..., then please use IncludeRules and ##Alert (look into other files how it's done).

These 'admonitions' are a different thing.
They are a feature of AsciiDoc, have to align to specific formats and are rendered in the output (see https://asciidoctor.org/docs/user-manual/#admonition).

Using alerts in comments is already supported the way you described.

Also, could you please change all itemData names to upper case naming? Eg. "replacement" -> "Replacement", or "section title" -> Section Title", etc? All other highlighting files use this naming scheme, and it will be visible in the UI later when configuring the colors in Kate.

Sure, no problem.

And what's still missing is a test file, also best licensed under MIT, that we can use for regression testing.

Working on it.
I understand a single AsciiDoc is expected?
I have a set of 22 AsciiDocs I used for testing while creating the highlighting support and another one describing the things I know are currrently not working.
At the moment I am merging them into a single one.

Thanks for your feedback :)

andreasgr updated this revision to Diff 50388.Jan 27 2019, 7:11 PM
  • Drop unused 'admonition' keyword list.
  • Rename itemData elements to start with capital letters.
  • Enable escaping in block and section titles.
andreasgr updated this revision to Diff 50390.EditedJan 27 2019, 7:21 PM

Add test AsciiDoc.

andreasgr updated this revision to Diff 50391.Jan 27 2019, 7:28 PM

Fix diff.

If you are only going to use the "admonition" keywords list for autocompletion, you could add them to the end of the context "section" or "start", keeping the attribute (Normal). I have done this in some highlight files

If you are only going to use the "admonition" keywords list for autocompletion, you could add them to the end of the context "section" or "start", keeping the attribute (Normal).

Thanks for the tip.
I think its not that important, though.

Hi!!
Please, also add a test file in: "autotests/input/". To generate the files in "folding", "html" and "reference" use make test and autotests/update-reference-data.sh.

I tried to understand how the testing works, but I failed.

make test complains about one test failed with latest master branch.
It complains about more failed tests after I added the AsciiDoc files.
I does not give hints about what's wrong, though.

Is there a guide somewhere about how to support testing of syntax highlighting?

dhaumann accepted this revision.EditedJan 28 2019, 9:41 PM
  1. I apply your patch: arc patch D18475
  2. Since there is a new syntax highlighting file, I do: touch data/syntax-data.qrc.in
  3. I switch to the build directory, type make -j4 install
  4. I run the tests, some fail, since the reference of your test file is missing
  5. In the build folder, I run: ./autotests/update-reference-data.sh
  6. Now git status tells me there are several untracked files:
    • autotests/folding/asciidoc.adoc.fold
    • autotests/html/asciidoc.adoc.html
    • autotests/reference/asciidoc.adoc.ref
  7. In the source folder, I git add all these files, then commit
  8. Finally, I call: arc land to land your patch
This revision is now accepted and ready to land.Jan 28 2019, 9:41 PM
This revision was automatically updated to reflect the committed changes.
  1. I apply your patch: arc patch D18475

Thanks for the information.
And many thanks for your time spent on reviewing.

Cheers