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).

In D18475#400238, @nibags wrote:

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.

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.

In D18475#400893, @dhaumann wrote:

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 :)

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

In D18475#400989, @nibags wrote:

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.

In D18475#400238, @nibags wrote:

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?

In D18475#401517, @dhaumann wrote:

1. I apply your patch: arc patch D18475

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

Cheers