Edgewall Software
Modify

Opened 2 years ago

Closed 12 months ago

Last modified 6 days ago

#10834 closed enhancement (fixed)

Option to add line numbers to code blocks

Reported by: Ryan J Ollos <ryan.j.ollos@…> Owned by: rjollos
Priority: normal Milestone: 1.1.2
Component: wiki system Version:
Severity: normal Keywords: wikimacros
Cc: ethan.jucovy@…, leho@…
Release Notes:

Line numbers and row highlighting can be specified for WikiProcessor code blocks.

API Changes:

The WikiProcessor argument parser supports supports dashes in the argument value.

Description

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.

Attachments (6)

t10834-r11295-1.patch (1.7 KB) - added by Ryan J Ollos <ryan.j.ollos@…> 2 years ago.
Patch against r11295 of trunk.
t10834-r11295-2.patch (2.5 KB) - added by Ryan J Ollos <ryan.j.ollos@…> 2 years ago.
Second revision of patch against r11295 of trunk.d
t10834-r11295-3.patch (4.3 KB) - added by Ryan J Ollos <ryan.j.ollos@…> 2 years ago.
Patch against r11295 of trunk.
t10834-r11421-1.patch (6.0 KB) - added by Ryan J Ollos <ryan.j.ollos@…> 2 years ago.
Patch against r11421 of the trunk.
WorkflowActionBox.png (16.4 KB) - added by Ryan J Ollos <ryan.j.ollos@…> 2 years ago.
t10834-20131008_014053.png (19.6 KB) - added by jomae 13 months ago.

Download all attachments as: .zip

Change History (51)

Changed 2 years ago by Ryan J Ollos <ryan.j.ollos@…>

Patch against r11295 of trunk.

comment:1 Changed 2 years ago by rblank

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.

comment:2 Changed 2 years ago by 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.

Changed 2 years ago by Ryan J Ollos <ryan.j.ollos@…>

Second revision of patch against r11295 of trunk.d

comment:3 Changed 2 years ago by 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.

comment:4 follow-up: Changed 2 years ago by cboos

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.

comment:5 Changed 2 years ago by 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?

comment:6 in reply to: ↑ 4 ; follow-up: Changed 2 years ago by rblank

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.

comment:7 in reply to: ↑ 6 Changed 2 years ago by cboos

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.

comment:8 Changed 2 years ago by 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?

comment:9 Changed 2 years ago by rblank

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

Changed 2 years ago by Ryan J Ollos <ryan.j.ollos@…>

Patch against r11295 of trunk.

comment:10 Changed 2 years ago by 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.

comment:11 Changed 2 years ago by 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.

comment:12 follow-ups: Changed 2 years ago by 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?).

comment:13 in reply to: ↑ 12 ; follow-up: Changed 2 years ago by cboos

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…

comment:14 in reply to: ↑ 12 Changed 2 years ago by 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.

comment:15 in reply to: ↑ 13 ; follow-up: Changed 2 years ago by 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.

comment:16 in reply to: ↑ 15 Changed 2 years ago by 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

comment:17 follow-up: Changed 2 years ago by cboos

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

comment:18 Changed 2 years ago by cboos

  • Milestone set to next-dev-1.1.x

comment:19 in reply to: ↑ 17 ; follow-up: Changed 2 years ago by 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.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 2 years ago by cboos

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.

comment:21 in reply to: ↑ 20 ; follow-up: Changed 2 years ago by 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.

Changed 2 years ago by Ryan J Ollos <ryan.j.ollos@…>

Patch against r11421 of the trunk.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 2 years ago by 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.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 2 years ago by cboos

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.

Changed 2 years ago by Ryan J Ollos <ryan.j.ollos@…>

comment:24 in reply to: ↑ 23 Changed 2 years ago by Ryan J Ollos <ryan.j.ollos@…>

  • Owner set to anonymous
  • Status changed from new to assigned

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:

comment:25 Changed 2 years ago by Ryan J Ollos <ryan.j.ollos@…>

  • Owner changed from anonymous to 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.

comment:26 Changed 21 months ago by 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?

comment:27 Changed 21 months ago by cboos

  • Keywords wikimacros added
  • Milestone changed from next-dev-1.1.x to 1.1.2

Yep.

comment:28 Changed 20 months ago by ethan.jucovy@…

  • Cc ethan.jucovy@… added

comment:29 Changed 14 months ago by rjollos

  • Owner changed from Ryan J Ollos <ryan.j.ollos@…> to rjollos

comment:30 Changed 13 months ago by rjollos

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

comment:31 Changed 13 months ago by rjollos

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
3print 'this is a python sample'
4a = b+3
5z = "this is a string"
6print 'this is the end of the python sample'


Line
3print 'this is a python sample'
4a = b+3
5z = "this is a string"
6print '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.

comment:32 Changed 13 months ago by rjollos

  • API Changes modified (diff)
  • Release Notes modified (diff)

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.

comment:33 Changed 13 months ago by rjollos

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.

comment:34 Changed 13 months ago by rjollos

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

comment:35 follow-up: Changed 13 months ago by rjollos

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

Changed 13 months ago by jomae

comment:36 follow-up: Changed 13 months ago by jomae

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.

comment:37 in reply to: ↑ 35 Changed 13 months ago by rjollos

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.

comment:38 in reply to: ↑ 36 ; follow-up: Changed 13 months ago by rjollos

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

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

  1. 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?

  1. {{{#!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.

comment:39 in reply to: ↑ 38 ; follow-up: Changed 13 months ago by jomae

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.

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

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

  1. {{{#!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.

comment:40 in reply to: ↑ 39 ; follow-up: Changed 13 months ago by rjollos

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.

comment:41 in reply to: ↑ 40 Changed 13 months ago by jomae

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.

comment:42 Changed 13 months ago by rjollos

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

An unintentionally change was fixed in [12185].

Last edited 13 months ago by rjollos (previous) (diff)

comment:43 Changed 12 months ago by rjollos

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:44 Changed 7 months ago by jomae

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.

comment:45 Changed 6 days ago by lkraav <leho@…>

  • Cc leho@… added

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed The owner will remain rjollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from rjollos to the specified user.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.