Add "toggle line numbers" JavaScript to source browser

[benhoyt]

At the moment when you copy-n-paste code snippets from Trac's source browser (I often do) it also copies the line numbers, which is almost never what you want. To get code without the line numbers, you have to scroll down and click the "Plain Text" version, find your place again, and then do the copy and paste.

It'd be nice if the Trac source file browser had a little JavaScript link, called "Toggle line numbers", that toggled the line numbers on and off, making copy-n-paste really clean and simple. (The toggle link could be placed in the table cell to the right of the "Line" heading cell at the top-left of the table.)

See the further discussion (agreement?) on the trac-users group: ​http://groups.google.com/group/trac-users/browse_frm/thread/71da6e9b5554b89c

It shouldn't be much code, especially with jQuery — Ted Gifford posted one possible solution: ​http://groups.google.com/group/trac-users/msg/846b21eb4e550e0a?dmode=source

Also see a somewhat related discussion on ticket:5779 about a "toggle line numbers" link in the diff viewer. They decided against it there, as normally you shouldn't need to copy-n-paste just a few lines of a diff or patch. (I find it's much more common with full source files, so believe the arguments against it for the diff viewer don't hold for the source viewer.)

BTW, not that I'd use it much, but I really like the operation of the "Tabular/Unified" button for the diff viewer in the new version of Trac. See an example of it at changeset:6599.

—-Ben Hoyt (benhoyt.com)

[Kamil Kisiel <kamil@…>]

I think that toggling the line numbers on and off for the purpose of copying kind of sucks. Take a look at the source browser on Google code. It displays line numbers, but when you select the code, the line numbers are not selected. You don't need to toggle them in order to copy.

[benhoyt]

Incidentally, I originally compared with Google Code too, and I'd prefer it that way as well. But as Matt Good said:

"This was considered a long time ago, but we opted in favor of making the source browser handle line-wrapping. The two options seem to be mutually exclusive. I've mentioned this trade-off to the Google Code developers since people complained a lot more when the lines did not wrap. You can always use the Plain Text link at the bottom to copy/ paste from. — Matt"

So apparently that's already been discussed, and I guess there's pros and cons with that method. I think the "Toggle line numbers" option is the next-best option. :-)

[benhoyt]

Oops, forgot to mention where Matt Good said that. On the trac-users list, here: ​http://groups.google.com/group/trac-users/msg/6ffca928f4a1d09a

[Christian Boos]

Expanding a bit on Ted Gifford's approach

[Christian Boos]

Cleaned-up version, feature is now also enabled for attachments

[Christian Boos]

Please have a look at attachment:hide_annotation_columns-r6692.2.diff​

[Christian Boos]

Just noticed a bug with 0pera (9.26 - Windows) when hiding both "Line" and "Rev" columns.

Works fine with Firefox 2.0.0.12, IE7, Safari 3.1.

[benhoyt]

Good work, cboos. I like open source. :-)

[Christian Boos]

Note also that with this available, I could add an Author column in the blame view, disabled by default (the reason why there's no Author so far is precisely because 3 annotations columns took too much space).

Besides, this is an enhancement, so not for 0.11.1.

[Christian Boos]

#2528 was closed since the ability to copy the source code without the line numbers will come as a bonus from this feature.

Btw, I have a fixed patch which works with Opera now, moving this to 0.12 as a reminder.

[ebray]

I've been using this patch for a while, and it works nicely for the most part. But I just noticed today that when annotation is enabled, clicking 'show line numbers' inserts the line numbers to the left of the annotation column instead of to the right.

[Christian Boos]

Latest version of the show/hide columns patch

[Christian Boos]

Erik, could you please try 7055-toggle-lines-r7656.patch​? It seems to preserve the ordering of the columns, unlike the previous versions.

[ebray]

Thanks! I was just about to propose a similar patch that I was just working on. Works much better now.

[Christian Boos]

Applied in [7674], thanks for the feedback!

[ebray]

Alas when testing this I never actually tested copy and pasting text with the new version. When the table columns are only hidden browsers still seem to copy the hidden text. I've only tested this in IE6 and FF3 in Windows and Linux, but they all the same problem. The hidden text being copied sort of defeats the purpose *sigh*.

[Christian Boos]

You're right, I just tested again: while it works fine in Chrome, Safari and Opera, it doesn't in IE7 and FF3…

[ebray]

Might be worth calling it a "browser bug" and leaving it as is. However, since the majority of my users are using IE (and no, I can't fix that as much as I wish I could) I came up with a fix in an attached patch.

It works more like the previous version in that it removes the ths and saves them in an array. The difference is that clicking on any column header removes all the columns. As clever as your solution is I think this makes more sense for the use case: The only reason I ever care to hide a column is for copy and paste purposes, and when I do that I hide every column anyways. Maybe a case could be made for wanting one column but not another, but until that case is made this solution works for me (that's just me though).

It's also a little slow on files > 1000 lines :/

[ebray]

New version that removes all columns simultaneously—this deals with the fact that IE and FF copy text from hidden table cells.

[ebray]

I should add that prior to this I tried an approach that was a combination of versions 1 and 2: It copied the <th> contents into an array and then hid each <th>. Worked okay in general, but again in IE and FF, each <th>, even though empty, caused a tab to be prepended to each line of the copied text.

[Christian Boos]

Seems to work fine for me, I have no problem with showing/hiding all the columns at once, it's more intuitive that way. As for the speed, that might be another incentive to switch your users to Chrome ;-)

Ideally, the faster solution (but with your UI simplification) should be kept for browsers supporting it, falling back to the slower but more robust solution for the other browsers.

[Christian Boos]

Ah, just noticed one problem with attachment:7055-toggle-lines-r7929.patch​, the annotation links won't work after collapse/show.

[ebray]

Replying to cboos:

Ah, just noticed one problem with attachment:7055-toggle-lines-r7929.patch​, the annotation links won't work after collapse/show.

Hm, right you are. You'd think this little issue shouldn't have to be this complicated. Hide/show would make the most sense if it weren't broken in a couple major browsers -_-;

I guess short of rebinding the click event to the new annotation links, the best approach might be to temporarily move those columns to some hidden table elsewhere in the DOM and then restore them. I'll have to try that out later.

[ebray]

Replying to ebray:

I guess short of rebinding the click event to the new annotation links, the best approach might be to temporarily move those columns to some hidden table elsewhere in the DOM and then restore them. I'll have to try that out later.

Yes, it looks like that should work. I'll submit a patch tomorrow maybe.

[Christian Boos]

I'm also considering another solution - get away from the one row per line model entirely and use the more light-weight 1 "fat" row with columns containing all the lines within a pre block. We would get rid of the selection problem altogether and that would also help a lot performance wise for big files. Of course, we would then loose line wrapping, but it's a trade-off worth considering. Google code, github, bitbucket to name a few all does it that way.

[ebray]

I rather prefer to keep the line wrapping, but I guess that's getting to be a religious issue, albeit a relatively minor one. Personally, I don't like really long lines breaking my page layout. Though that would be solvable by putting code in a div with fixed width and overflow:scroll. How important is line wrapping? Does it really matter that much?

Perhaps it could be put to a vote with the users.

[Christian Boos]

Alternative approach, generating one unique row with columns containing <pre> elements and all the lines in div blocks

[Christian Boos]

For experimenting and feedback, I attach a proof-of-concept patch. It's not yet pretty for blames, but marks work (the 'hilite' class).

Though it looks mostly fine in IE, copy/paste will miss the newlines ;-) I don't know yet if this can be easily fixed, as it seems that other code renderers (e.g. github) have the same issue.

[Christian Boos]

The change should also benefit to #5499.

[Christian Boos]

Not critical for the release.

[Carsten Klein <carsten.klein@…>]

I also find this annoying. However, for a recent project of mine I have "developed" a perl script that would separate both line numbers and line content into two different divs.

By that, you are able to select the actual text without also selecting the line numbers.

The solution is rather simple:

• collect all line numbers in a single div and collect all line content in a different div.

'<div class="lineno">' . join( '<br>', @linenos ) . '</div>' . '<div class="line">' . join( '<br>', @lines ) . '</div>';

• however, it will not allow line content to be broken into several lines conforming to the same lineno
• instead it requires that the source view has a style of overflow:auto so that a scrollbar will be diplayed once the browser window becomes too small to render the line content

• making this more versatile and also adding the ability to flow line content across multiple lines whilst also maintaining the correlation between line number and line seems very complicated

[Carsten Klein <carsten.klein@…>]

i would propose this as won't fix and pointing out the fact that one can always also download/view the plain text version.

see also ​http://trac.edgewall.org/ticket/7055#comment:2

and mark this and the previous comment as redundant ;)

[kamil@…]

I would prefer if this worked more like the Google Code style browser wherein the selection selects only the code, and not have the numbers be toggled on and off. It looks like the way they implement this is by having two tables, one for the text data and one for the line numbers.

[mike.schall@…]

SyntaxHighlighter looks like a library that does what is suggested here.

​http://alexgorbatchev.com/SyntaxHighlighter/whatsnew.html

[walty <walty8@…>]

hi,

Please find my in-progress patch in ​32f28d0d, and I would like to explain a bit for it.

The main problem I got is that, during copy and paste of content from the diff page (revision diff or changeset display), the line numbers are always included. The source code browser page fixed it by hiding line numbers, but the feature is not available in the diff page.

I previously tried to use ​user-select property to disable the line number selection, but it does not work well for chrome. It disables the line number selection, but you actually still copied them, as illustrated ​here.

Finally I checked the source code of ​GitHub diff page, which supports both line wrap and skip of line numbers. Turns out that they used css to display the line numbers.

For example, the following combination would display the line number of 124 that cannot be selected.

HTML:

<th trac-line-number='124'></th>

CSS:

th[trac-line-number]:before { content: attr(trac-line-number); }

Please advise if the approach in the ​patch is fine, if no problem, I would wrap up the unit tests part.

Thanks.

[Jun Omae]

I don't read the patch. However, Trac currently uses XHTML 1.0 Strict. trac-line-number attribute cannot be used under the DTD. At least, we should use data attribute or data- prefix attribute if we use HTML5 after we migrate Genshi to Jinja2.

p.s. <th trac-line-number="&nbsp;"> in the patch is incorrect.

[walty <walty8@…>]

hi,

Please find the latest patch in ​4ea3e0c. The attribute naming is updated, and this patch also handled the hellip of diff page.

Here is the result of selection before the patch:

And this is the selection effect after the patch:

Note that for the side-by-side selection, one may still accidentally select the content of the other region. Like the following:

But I checked github.com, and they seem to have the same problem. I guess it should not be a major problem.

[Ryan J Ollos]

The changes work well for me. The changes will need to be rebased against the Jinja2 branch if they are to be committed after that work is pushed to the trunk.

[Christian Boos]

Also, I wonder if a similar approach could be used for the source browser, maybe coupled with a CSS counter.

[Christian Boos]

e.g. [8527d32b/cboos.git], tested with Chrome.

One issue with this is that empty lines disappear from the selection, but that's already the case with the current solution anyway.

[Christian Boos]

Replying to Christian Boos:

One issue with this is that empty lines disappear from the selection

With Firefox (47.0.1), copy/paste even keeps the empty lines.

OTOH, the whole CSS approach, for changesets and source browser, doesn't work with IE11 and Edge (i.e. copy/paste keeps the line numbers). I'd say we don't care.

[Jun Omae]

Replying to Christian Boos:

Replying to Christian Boos:

One issue with this is that empty lines disappear from the selection

#11445 is similar.

With Firefox (47.0.1), copy/paste even keeps the empty lines.

I tried cboos.git@7055.2 with trunk/Makefile​ and Pygments:

• Firefox 47.0.1 (Windows 7): TAB character is inserted to begin of each line in the copied text
• Chrome 52.0 beta: empty lines disappear from the copied text

[Christian Boos]

Thanks for testing!

I tried with the extra tag.br from #11445:

• Firefox 47.0.1 (Windows 7): TAB character is inserted to begin of each line in the copied text

No differences.

• Chrome 52.0 beta: empty lines disappear from the copied text

(51.0.x here) Empty lines are kept, no tabs. There can also a tab at the beginning of the first pasted line, if one selected the first line number instead of the content of the first line. That's easy to work around.

As for Firefox, I just found out that you can CTRL+click/drag to select particular cells in a table. That way, it is easy to select only the source cells, and copy/pasting preserves the empty lines and doesn't have the leading tab.

What remains is to find a way to support code snippets with the lineno parameter (r12184).

[Jun Omae]

Replying to Christian Boos:

I tried with the extra tag.br from #11445:

• Firefox 47.0.1 (Windows 7): TAB character is inserted to begin of each line in the copied text

No differences.

I tried to solve with user-select attribute in [43def54a5/jomae.git]. It works fine with Firefox and Chrome.

[walty <walty8@…>]

Replying to Jun Omae:

I tried to solve with user-select attribute in [43def54a5/jomae.git]. It works fine with Firefox and Chrome.

The problem with user-select is that, inside Chrome, though you text is not highlighted during selection, but they can still be copied and pasted. ​This site provides some more details about this one.

[Jun Omae]

Replying to walty <walty8@…>:

Replying to Jun Omae:

I tried to solve with user-select attribute in [43def54a5/jomae.git]. It works fine with Firefox and Chrome.

The problem with user-select is that, inside Chrome, though you text is not highlighted during selection, but they can still be copied and pasted. ​This site provides some more details about this one.

Yes. However, use of :before { content: ... } avoids line numbers in text selection for Chrome. I confirmed clipboard after copying all text (CTRL-A + CTRL-C) doesn't include line numbers with Chrome.

Use of -moz-user-select: none for tbody th.linenum in [43def54a5/jomae.git] avoids TAB characters appear at begin of lines in text selection for Firefox.

[walty <walty8@…>]

Replying to Jun Omae:

Yes. However, use of :before { content: ... } avoids line numbers in text selection for Chrome. I confirmed clipboard after copying all text (CTRL-A + CTRL-C) doesn't include line numbers with Chrome.

Use of -moz-user-select: none for tbody th.linenum in [43def54a5/jomae.git] avoids TAB characters appear at begin of lines in text selection for Firefox.

I see, it's a mix of two approaches. Sorry that I overlooked it in the code.

[walty <walty8@…>]

Replying to Christian Boos:

Also, I wonder if a similar approach could be used for the source browser, maybe coupled with a CSS counter.

The only concern was that the browser page already provided a toogle line number button to avoid line number selection. So the CSS to skip line number selection seems duplicate.

It may be more consistent to use the same style for the source code display, however.

Please advise if it's preferred. I would help set up a new patch based on jinja2, integrating the recommended fix from Jun Omae for Firefox, and probably include some updates of the unit tests.

Thanks.

[Christian Boos]

Replying to 48:

What remains is to find a way to support code snippets with the lineno parameter (r12184).

Added in cboos.git@7055.3. We still have to fix the mimeview tests, but we can do that last when things have settled.

I also wanted to get rid of the initial solution (enableCollapsibleColumns), but that's still somehow useful in blame mode…

(PS: nitpick about r12184, discarded was the correct spelling, AFAICT ;-) )

[Ryan J Ollos]

r12184 accidentally reverted r12183. r12183 was re-applied in r12185.

[Christian Boos]

Replying to walty <walty8@…>:

Please advise if it's preferred. I would help set up a new patch based on jinja2,

As the jinja2 branch itself has to be rebased on current trunk, I'd suggest to rebase the series after that.

integrating the recommended fix from Jun Omae for Firefox, and probably include some updates of the unit tests.

Well, for updating the tests, you can simply continue my branch (or a Jun's next iteration if there's one!).

For example, assuming you have a "cboos" remote (as explained in TracTeam/Repositories):

$ git fetch cboos 7055.3
$ git checkout -b 7055.4 cboos/7055.3

(TODO we need to restore public access to ​https://svn.edgewall.org/git/…)

[Christian Boos]

Replying to Ryan J Ollos:

r12184 accidentally reverted r12183. r12183 was re-applied in r12185.

Ah my bad! And the original misspelling can even be blamed back to… me, 9 years ago ;-)

[Ryan J Ollos]

Milestone renamed