Edgewall Software
Modify

Opened 13 years ago

Closed 11 years ago

#4339 closed defect (fixed)

PygmentsRenderer produces unnecessary spans

Reported by: Tim Hatch <trac@…> Owned by: Christopher Lenz
Priority: high Milestone: 0.11
Component: version control/browser Version: devel
Severity: normal Keywords: mimeview highlighting
Cc: Branch:
Release Notes:
API Changes:

Description

On trunk post-r4399, it's possible (and likely) to end up with <span class=""> in the rendered source (I think the Pygments plain text one does this for everything).

I think this is due to pygments.formatters.html.HtmlFormatter._get_css_class (source:trunk/trac/mimeview/pygments_renderer.py@4399#L219) returning an empty string.

Perhaps it's returning the wrong class, but if indeed it's supposed to be the empty string, then I suggest the span not be output?

Attachments (2)

t4339.diff (6.4 KB ) - added by Tim Hatch <trac@…> 13 years ago.
t4339b.diff (3.2 KB ) - added by Tim Hatch 13 years ago.
New patch, closer to the problem, as of r4575

Download all attachments as: .zip

Change History (11)

comment:1 by Christopher Lenz, 13 years ago

Keywords: mimeview highlighting added
Milestone: 0.11
Owner: changed from Christian Boos to Christopher Lenz
Status: newassigned

Thanks, I'll look into this.

comment:2 by Tim Hatch <trac@…>, 13 years ago

I did some quick tests to eradicate:

  • <span class="something-valid"></span> at beginning and end of line
  • <span class="">some-stuff</span>some-stuff

The first is generated for most lines because a span isn't closed till after the \n and is thus opened and immediately closed a lot.

The generated size for a reasonably large test (source:trac/mimeview/tests/api.py@3544 as of Nov 10) rendered using a /file link on my test Trac, the filesize dropped from 31078 bytes to 22966 bytes (the plaintext source is 3.3k and the chrome overhead appears to be about 6.4k). This makes the bloat from syntax highlighting (this specific file) go from 7.4x in trunk to 5.0x with this patch.

I've confirmed that they are both using PygmentsRenderer. The only visual difference appears to be the extra blank line for files with \n at EOF (see comment:ticket:4336:6 — it's due to other changes).

I'm attaching the patch in a second, welcoming comments. I don't see that there is a large overhead for doing this processing, and it does make the files quite a bit smaller.

P.S. the patch is on top of the one for #4336 and modifies some of its tests.

P.P.S. If you can't view older revs of api.py on t.e.o, see #4365

by Tim Hatch <trac@…>, 13 years ago

Attachment: t4339.diff added

comment:3 by Christian Boos, 13 years ago

Is this still needed as of Pygments:0.6?

If I'm not mistaken, the empty spans are gone.

(btw, jonas, could you add an InterTrac prefix for pygments?)

comment:4 by Tim Hatch, 13 years ago

Check source:trunk/trac/mimeview/tests/pygments.html on line 17 (and others). Although those tests don't appear to be passing on my system, the part about expecting silly spans is true. When that's passed through _group_lines you end up with empty spans at the beginning or end of line.

I just ran api.py through it again and it came up with this snippet:

...'text/plain'...

which actually has another issue too, closing and immediately reopening in the middle of the line.

comment:5 by Tim Hatch, 13 years ago

sorry, that snippet is supposed to be

...<span class="s">'</span><span class="s">text/plain</span><span class="s">'</span>...<span class=""></span>

There we go again with the default rendering vs syntax coloring :)

comment:6 by Christian Boos, 13 years ago

Well, the highlighter to use in this case is #!xml:

...<span class="s">'</span><span class="s">text/plain</span><span class="s">'</span>...<span class=""></span>

About the pygment tests: yes, you're right, I overlooked the fact that the empty spans are still there. You can update the "expected" content, if you think that the pygmentized output is now "stable".

by Tim Hatch, 13 years ago

Attachment: t4339b.diff added

New patch, closer to the problem, as of r4575

comment:7 by Tim Hatch, 13 years ago

Ok, I've got a new patch, see attachment:t4339b.diff

I've got the spans outputting the way I want, just by combining text nodes and actually checking for a null css class instead of Attrs… because Attrs([('class', '')]) doesn't evaluate to False.

The tests are all failing though, because whitespace is being munged. There are a couple of debug statements added to _test() to aid in this.

comment:8 by Christian Boos, 11 years ago

Milestone: 0.11.10.11
Priority: normalhigh

Bumping up the priority of this. Anything that can help reduce the memory usage is good :-)

Tim, if you could check/revive this patch, maybe we can get that in 0.11?

comment:9 by Christian Boos, 11 years ago

Resolution: fixed
Status: assignedclosed

Updated patch committed as r7057, plus another optimization committed as r7058.

With both optimizations, the highlighted page size can be only 60% of the page size it used to be! (e.g. for some old version of trac/attachment.py, 24.2 kB of Python code, the size of the corresponding highlighted document is now 125 kB and used to be 215 kB).

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christopher Lenz.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Christopher Lenz to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.