Option to add line numbers to code blocks

[Ryan J Ollos <ryan.j.ollos@…>]

Inspired by ​th:LinenoMacro, the attached patch adds an option annotations=lineno that adds line numbers to code blocks.

{{{#!python annotations=lineno
}}}

The unit test is failing. I'm hoping someone can give me some guidance on that.

[Ryan J Ollos <ryan.j.ollos@…>]

Patch against r11295 of trunk​.

[Remy Blank]

I'm not familiar with annotations in the mimeview renderers, but it looks like this would generate duplicate element IDs if there are two such blocks on the same page.

[Ryan J Ollos <ryan.j.ollos@…>]

Yeah, your are right, it does generate duplicate element IDs. It seems like the easiest thing to do would be to just drop the element IDs from the line numbers. This was actually the approach taken by the ​th:LinenoMacro before I changed it to use trac.mimeview.api.LineNumberAnnotator.

[Ryan J Ollos <ryan.j.ollos@…>]

Second revision of patch against r11295 of trunk​.d

[Ryan J Ollos <ryan.j.ollos@…>]

I'm not sure that modifying LineNumberAnnotator is the best way to accomplish this, but that is what t10834-r11295-2.patch​ does to try and fix the issue. It might be better to add another implementation of IHTMLPreviewAnnotator.

[Christian Boos]

Good suggestion with this ticket! However I'd prefer something like lineno=n (then start numbering at n). And if we pass id=something, then we could also have the #somethingLn links.

[Ryan J Ollos <ryan.j.ollos@…>]

I didn't like the verboseness of annotations=lineno anyway, so lineno=n sounds like a good alternative. So do you think I should take the route of implementing a new IHTMLPreviewAnnotator?

[Remy Blank]

Replying to cboos:

And if we pass id=something, then we could also have the #somethingLn links.

Users shouldn't have to worry about Trac generating well-formed and validated XHTML. We should implement an auto-numbering scheme like we do for heading anchors. The id= attribute could still be useful, in the same way as we can override the heading anchors, but we should do the right thing automatically by default.

[Christian Boos]

For a code block, if you just autonumber it, the links will break as soon as you're prepending another block. Having to specify id= explicitly for having anchors makes it less likely to shoot yourself in the foot. But OK, as the links would probably be placed just next to the code snippet anyway, this will not be very consequential.

[Ryan J Ollos <ryan.j.ollos@…>]

I've attempted to implement the unique autonumbered ids for the ​th:LinenoMacro. The ids are of the form a\d-L\d (​th:browser:/linenomacro/trunk/lineno/LinenoMacro.py@11954). When the id attribute is specified, the anchor id would be %s-L\d % id. Does that seem like an okay scheme?

[Remy Blank]

Yes, looks good.

The auto-numbering of anchors ensures that our XHTML always validates, and it's fine for the cases where the anchors are not actually used, or when there's only one code snippet. When there's more and you break your links, it's easy to fix with id=.

[Ryan J Ollos <ryan.j.ollos@…>]

Patch against r11295 of trunk​.

[Ryan J Ollos <ryan.j.ollos@…>]

t10834-r11295-3.patch​ implements the proposed feature.

• If id is not specified, the first unique anchor id prefix is chosen, starting with a1 and incrementing the digit.
• Line numbers are only added to the code block if the lineno option is specified. Specifying the id option without specifying lineno has no effect.

Also:

• Removed an unnecessary inline import from trac.mimeview.api import Mimeview.
• Fixed an obsolete comment.
• Added a new Formatter class member function _unique_anchor.

Some undesirable behavior exists. If there is a space added between the key/value pair, for example id = b, then the arguments become id=True, b=True, and the anchor id prefix becomes True. This behavior effects any key/value pairs passed to WikiProcessors, and it looks like WikiProcessors#UsingProcessors is fairly clear about the syntax, but we may want to improve on this in the future and strip whitespace around the = character.

[Ryan J Ollos <ryan.j.ollos@…>]

I wasn't sure whether passing data through the hints member variable was the right thing to do here, but I didn't see another way to pass data to the IHTMLPreviewAnnotator within the existing structure. I'll modify the patch if someone points me in another direction.

[Christian Boos]

It's fine, it's just that the term hints is a bit over-restrictive here. I meant to try out the patches soon (or the patch, it's just t10834-r11295-3.patch​, right?).

[Christian Boos]

As a side-note:

t10834-r11295-3.patch​, right?).

Oops, [attachment:t10834-r11295-3.patch​]/t10834-r11295-3.patch​ vs. [attachment:t10834-r11295-3.patch]/t10834-r11295-3.patch​ (i.e. without the trailing ZWSP) …

I thought we fixed that…

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to cboos:

It's fine, it's just that the term hints is a bit over-restrictive here. I meant to try out the patches soon (or the patch, it's just t10834-r11295-3.patch​, right?).

Yes, just t10834-r11295-3.patch​. It supersedes the other patches.

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to cboos:

I thought we fixed that…

Jun fixed a similar case for the Image macro (#10668). I had been wondering whether the same problem could come about with attachment TracLinks, but hadn't been able to reproduce what you've just shown. I could put together a fix similar to [11074], if no one wants to beat me to it. I guess a new ticket is in order.

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to Ryan J Ollos <ryan.j.ollos@…>:

Jun fixed a similar case for the Image macro (#10668). I had been wondering whether the same problem could come about with attachment TracLinks, but hadn't been able to reproduce what you've just shown. I could put together a fix similar to [11074], if no one wants to beat me to it. I guess a new ticket is in order.

⇒ #10865

[Christian Boos]

Btw, I just noticed we had a ticket for that already, #3275. Some interesting ideas there, for example the link to Sphinx.

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to cboos:

Btw, I just noticed we had a ticket for that already, #3275. Some interesting ideas there, for example the link to Sphinx.

Are you hinting that we should utilize Pygments, or did you like some of the other options offered by Sphinx such as emphasize-lines?

I have some time to do more work on the patch over the weekend. Maybe I can figure out what is wrong with the unit test.

[Christian Boos]

Replying to Ryan J Ollos <ryan.j.ollos@…>:

Replying to cboos:

Btw, I just noticed we had a ticket for that already, #3275. Some interesting ideas there, for example the link to Sphinx.

Are you hinting that we should utilize Pygments, or did you like some of the other options offered by Sphinx such as emphasize-lines?

The latter, of course. An equivalent to emphasize-lines could be achieved with mark=120,123-125 and reuse the marking mechanism we already have.

I have some time to do more work on the patch over the weekend. Maybe I can figure out what is wrong with the unit test.

Well, the unit-test wasn't refreshed since the first patch it seems, so it doesn't have lineno or lineno=12 (two or more tests, also with id=explicit-id, would be even better).

But the patch looks good to me.

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to cboos:

The latter, of course. An equivalent to emphasize-lines could be achieved with mark=120,123-125 and reuse the marking mechanism we already have.

I've added the marks option to the revised patch. One ugly and probably error-prone aspect of this is that it seems to be necessary to quote the values, for example: marks="120,123-125". One first glance, it looks like some changes to trac.wiki.parse.parse_processor_args would be required to change this behavior.

I'll add some tests in the next iteration of the patch, but just wanted to post the latest changes for review, in case there is anything obvious that can be spotted.

[Ryan J Ollos <ryan.j.ollos@…>]

Patch against r11421 of the trunk.

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to Ryan J Ollos <ryan.j.ollos@…>:

I've added the marks option to the revised patch. One ugly and probably error-prone aspect of this is that it seems to be necessary to quote the values, for example: marks="120,123-125". One first glance, it looks like some changes to trac.wiki.parse.parse_processor_args would be required to change this behavior.

Rather than providing another patch, I pushed the latest changes to a branch of my fork in ​936367a44adf4c08ecd4391f699fb16e3fe5f260. Please bear with me while I learn how to use Git, and I appreciate any hints on how to do this better.

I propose the follow change to parse_processor_args:

-PROCESSOR_PARAM = r'''(?P<proc_pname>\w+)=(?P<proc_pval>".*?"|'.*?'|\w+)'''
+PROCESSOR_PARAM = r'''(?P<proc_pname>\w+)=(?P<proc_pval>".*?"|'.*?'|[-,\w]+)'''

Here is the before/after output:

>>> parse_processor_args('ab=c de -f gh="ij klmn" p=q-r')
{'ab': 'c', 'f': False, 'de': True, 'p': 'q', 'r': False, 'gh': 'ij klmn'}

>>> parse_processor_args('ab=c de -f gh="ij klmn" p=q-r')
{'p': 'q-r', 'de': True, 'ab': 'c', 'f': False, 'gh': 'ij klmn'}

and p=q-r,s yields the dictionary entry 'p': 'q-r,s'.

This would allow the marks=120,123-125 syntax to be accepted by the processor. I implemented this in ​84ba753c5185c2182f3eb59957c5f4c30fe080e5.

Test cases have been added. There were a two issues that could probably be improved on:

• The .replace(u'\\xa0',u'\xa0') was a workaround for not knowing how to deal with the unicode escape sequence being escaped on read.
• There is no line break after the table, so I couldn't allow a line break between the expected output and the 30 character test case delimiter.

Other than that, the patch is ready in my opinion, though I'll do more work on it if I get feedback.

[Christian Boos]

Replying to Ryan J Ollos <ryan.j.ollos@…>:

Rather than providing another patch, I pushed the latest changes to a branch of my fork in ​936367a4. Please bear with me while I learn how to use Git, and I appreciate any hints on how to do this better.

Great! First tip: 8 digits for referring to commits are (more than) enough.

Second tip, there are basically two ways to work on topic branches:

1. cumulative: add new changesets to integrate feedback; the end result will get committed back in the main branch (or svn, for us) as a whole anyway, so the precise content of each changeset doesn't matter that much
2. iterative: rework each changeset until each change is "perfect"; the end result might be committed 1 to 1 to the main branch, as a way to better document the changes

Choose whichever style you prefer or is appropriate to the nature of the feature. The "cumulative" style is certainly easier and the "iterative" style is really great if you like to work that way (it's very similar to working with MQ, in Mercurial), but is a bit harder to get used to (rebase -i is your friend!). And there is the advice: if you choose this way, never reuse the same branch name for a rebased branch, but start by making a copy (.2 suffix), then rebase that copy. See for example #10717. That advice also applies if for some reason you feel you need to rebase your branch on top of the latest changes from upstream.

I propose the follow change to parse_processor_args: […] and p=q-r,s yields the dictionary entry 'p': 'q-r,s'.

After thinking a bit about it, I wasn't able to find a counter example for the comma, so I think I'm fine with this.

Test cases have been added. There were a two issues that could probably be improved on:

• The .replace(u'\\xa0',u'\xa0') was a workaround for not knowing how to deal with the unicode escape sequence being escaped on read.

Just insert a plain unicode "NO-BREAK SPACE" character in the .txt file instead. (Hint: C-x 8 RET a0, or copy/paste from the French translation file which is full of these little things if you don't use the One True Editor)

• There is no line break after the table, so I couldn't allow a line break between the expected output and the 30 character test case delimiter.

Add "Paragraph" after the block. That way the generated content will end with:

...</table><p>
Paragraph
</p>

It's a trick we use for a lot of other tests there.

Other than that, the patch is ready in my opinion, though I'll do more work on it if I get feedback.

A part from improving the tests, I also think it's (nearly) good to go (I said nearly as I just added a few inline comments about TracDev/CodingStyle, on the changesets themselves on GitHub). I'm quite pleased with the result, great job!

Note: you can "assign" the ticket to yourself, it will then show up in the assigned group in the milestone progress bar (in yellow), which is helpful for showing work-in-progress tickets.

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to cboos:

[…] A part from improving the tests, I also think it's (nearly) good to go (I said nearly as I just added a few inline comments about TracDev/CodingStyle, on the changesets themselves on GitHub). I'm quite pleased with the result, great job!

Thanks for the review and feedback. I think I've captured these changes in ​8343e40d.

Note: you can "assign" the ticket to yourself, it will then show up in the assigned group in the milestone progress bar (in yellow), which is helpful for showing work-in-progress tickets.

I'll give this a try, but even though I have my username and email set in the session data, it is showing that the owner will be changed to anonymous:

[Ryan J Ollos <ryan.j.ollos@…>]

It looks like I can change ownership to now, but I'll have to manually input my username and email.

[Ryan J Ollos <ryan.j.ollos@…>]

Would it be okay to target this one to 1.1.2 since it has a patch that has been reviewed?

[Christian Boos]

Yep.

[Ryan J Ollos]

Unrelated changes from ​t10834 on GitHub committed to 1.0-stable in [12111] and merged to trunk in [12112].

[Ryan J Ollos]

There is a problem with the patch, which I'll propose a fix for shortly, though I'm not certain that my fix is the right way to go yet. I'll be hoping to get some feedback. The problem is exhibited with the following markup:

{{{#!python lineno=3 id=b marks=4-5
print 'this is a python sample'
a = b+3
z = "this is a string"
print 'this is the end of the python sample'
}}}

[[BR]]

{{{#!python lineno=3 id=d
print 'this is a python sample'
a = b+3
z = "this is a string"
print 'this is the end of the python sample'
}}}

which renders as:

Line
3	print 'this is a python sample'
4	a = b+3
5	z = "this is a string"
6	print 'this is the end of the python sample'

Line
3	print 'this is a python sample'
4	a = b+3
5	z = "this is a string"
6	print 'this is the end of the python sample'

Note that the second WikiProcessor call doesn't have the marks argument, but still highlights the rows.

[Ryan J Ollos]

Proposed changes can be found in repos:rjollos.git:t10834. To deal with the issues described in comment:31:

• A child RenderingContext is created in _mimeview_processor before adding the hints.
• The hilite class is no longer added in _render_source when marks have been specified as request arguments. The hilite class is added by LineNumberAnnotator. This appears to work just fine for rendering in the source browser as well.

[Ryan J Ollos]

I was encountering some tracebacks when lineno was not an integer, and marks was not a valid range. Those issues should be fixed in log:rjollos.git:t10834.2.

[Ryan J Ollos]

Does it make sense to try to move as much of the computation as possible into get_annotation_data since it is only called once? That is what I've done in [7b7a6de3/rjollos.git].

[Ryan J Ollos]

… or just move the computation into _mimeview_processor: [fbb7d0a4/rjollos.git].

[Jun Omae]

I just tried [fbb7d0a4/rjollos.git], see attachment:t10834-20131008_014053.png​​.

1. lineno and marks parameter accept a negative integer and zero. I think we should ignore such integers.
2. The code block with line numbers has no margin.
3. If {{{#!diff lineno}}} blocks are repeated, the second diff block doesn't render as diff.
4. {{{#!default}}} processor ignores lineno parameter. If the code block has no processor name, there is no way to specify lineno parameter. Therefore, use {{{#!default}}} processor. However, the processor ignores the parameter.

[Ryan J Ollos]

Replying to rjollos:

… or just move the computation into _mimeview_processor: [fbb7d0a4/rjollos.git].

Oh, that does not work since it breaks rendering in the Repository Browser.

[Ryan J Ollos]

Replying to jomae:

I just tried [fbb7d0a4/rjollos.git], see attachment:t10834-20131008_014053.png​​.

Thank you for the feedback. The latest changes can be found in log:rjollos.git:t10834.3.

1. lineno and marks parameter accept a negative integer and zero. I think we should ignore such integers.

This should be fixed in [c28b3394/rjollos.git].

2. The code block with line numbers has no margin.

I tried to fix this in [8e74590f/rjollos.git], but I won't be surprised if there is a better way. I couldn't just add a margin to table.code since that would affect tables rendered in the Repository Browser, so the only way I could see to solve the problem was to put a div around the table, and I did the same with div.code elements for consistency.

3. If {{{#!diff lineno}}} blocks are repeated, the second diff block doesn't render as diff.

I haven't seen any issues with the unified diffs, for which the PatchRenderer will be used, nor would I expect the lineno argument to be used in this case. For contextual diffs, when the lineno argument is passed the lines are numbered sequentially with only one column of numbers. It would be nice to render these with two columns of line numbers like unified diffs, but I'm not sure that should really be in the scope of this ticket. It might be an enhancement for the PatchRenderer.

Either way, I'm not sure that I'm looking at the same issue you refer to. Can you post the markup that demonstrates the problem?

4. {{{#!default}}} processor ignores lineno parameter. If the code block has no processor name, there is no way to specify lineno parameter. Therefore, use {{{#!default}}} processor. However, the processor ignores the parameter.

I hadn't considered this to be a relevant use case, but I suppose it could be. Rather than rendering the text in pre element, we'll need to use the Mimeview renderer. I attempted to fix this in [900352d2/rjollos.git]. The commented out line is an attempt to avoid calling Mimeview.get_mimetype since we already know the mimetype and just need the charset, however it's probably not much of an optimization so I might just remove it.

[Jun Omae]

Verified log:rjollos.git:t10834.3.

1. lineno and marks parameter accept a negative integer and zero. I think we should ignore such integers.

This should be fixed in [c28b3394/rjollos.git].

It works fine.

2. The code block with line numbers has no margin.

I tried to fix this in [8e74590f/rjollos.git], but I won't be surprised if there is a better way. I couldn't just add a margin to table.code since that would affect tables rendered in the Repository Browser, so the only way I could see to solve the problem was to put a div around the table, and I did the same with div.code elements for consistency.

Your fix is good and simple to me.

3. If {{{#!diff lineno}}} blocks are repeated, the second diff block doesn't render as diff.

I haven't seen any issues with the unified diffs, …

Sorry, I've used invalid diff contents. Please ignore it.

4. {{{#!default}}} processor ignores lineno parameter. If the code block has no processor name, there is no way to specify lineno parameter. Therefore, use {{{#!default}}} processor. However, the processor ignores the parameter.

I hadn't considered this to be a relevant use case, but I suppose it could be. …

I've thought that it uses #!default lineno if showing plain text with line numbers because #!default is used if showing plain text. Okay. #!text lineno and #!text/plain lineno processor have the same result.

[Ryan J Ollos]

Replying to jomae:

I've thought that it uses #!default lineno if showing plain text with line numbers because #!default is used if showing plain text. Okay. #!text lineno and #!text/plain lineno processor have the same result.

Do you think then that we should include the change [900352d2/rjollos.git]? As far as I can see, I think it is good, and useful to be able to use lineno with #!default, #!text and #!text/plain.

Thank you for testing! There would have been quite a few bugs without your input.

[Jun Omae]

Replying to rjollos:

Do you think then that we should include the change [900352d2/rjollos.git]? As far as I can see, I think it is good, and useful to be able to use lineno with #!default, #!text and #!text/plain.

The changes look good to me! I think we should include it, that would be consistent.

[Ryan J Ollos]

Committed to trunk in [12184]. I'll add some documentation to the WikiProcessors page before closing this out.

[Ryan J Ollos]

I added a brief bit of documentation. I'll have to wait until t.e.o is upgraded to at least r12185 before uncommenting the example. I think we'll have to wait for the 1.2 release anyway, since it appears that the documentation is updated for maintenance releases: [11647].

[Jun Omae]

After [12615], unit tests for this ticket failed because #!python in wiki-tests.txt is highlighted if pygments is enabled. The issue has been fixed in [12619]. See comment:12:ticket:10838.