Edgewall Software
Modify

Opened 11 years ago

Last modified 2 weeks ago

#7055 new enhancement

Add "toggle line numbers" JavaScript to source browser

Reported by: benhoyt Owned by:
Priority: normal Milestone: next-major-releases
Component: rendering Version: devel
Severity: normal Keywords: javascript performance patch
Cc: benhoyt@…, Noah Kantrowitz, trac@…, Ryan J Ollos, walty8@… Branch:
Release Notes:
API Changes:

Description

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)

Attachments (9)

hide_annotation_columns-r6692.diff (3.7 KB ) - added by Christian Boos 11 years ago.
Expanding a bit on Ted Gifford's approach
hide_annotation_columns-r6692.2.diff (4.6 KB ) - added by Christian Boos 11 years ago.
Cleaned-up version, feature is now also enabled for attachments
7055-toggle-lines-r7656.patch (4.6 KB ) - added by Christian Boos 11 years ago.
Latest version of the show/hide columns patch
7055-toggle-lines-r7929.patch (3.0 KB ) - added by ebray 11 years ago.
New version that removes all columns simultaneously—this deals with the fact that IE and FF copy text from hidden table cells.
t7055-alt-r7933.patch (2.9 KB ) - added by Christian Boos 11 years ago.
Alternative approach, generating one unique row with columns containing <pre> elements and all the lines in div blocks
inline-after.png (19.3 KB ) - added by walty <walty8@…> 3 years ago.
inline-after.2.png (19.3 KB ) - added by walty <walty8@…> 3 years ago.
in-line-before.png (19.8 KB ) - added by walty <walty8@…> 3 years ago.
side-by-side.png (19.0 KB ) - added by walty <walty8@…> 3 years ago.

Download all attachments as: .zip

Change History (67)

comment:1 by Kamil Kisiel <kamil@…>, 11 years ago

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.

comment:2 by benhoyt, 11 years ago

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

comment:3 by benhoyt, 11 years ago

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

comment:4 by Noah Kantrowitz, 11 years ago

Cc: Noah Kantrowitz added

by Christian Boos, 11 years ago

Expanding a bit on Ted Gifford's approach

by Christian Boos, 11 years ago

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

comment:5 by Christian Boos, 11 years ago

Keywords: review added

comment:6 by Christian Boos, 11 years ago

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.

comment:7 by benhoyt, 11 years ago

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

comment:8 by trac@…, 11 years ago

Cc: trac@… added

comment:9 by Christian Boos, 11 years ago

Milestone: 0.11.10.12

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.

comment:10 by Christian Boos, 11 years ago

Milestone: 0.130.12
Owner: changed from Jonas Borgström to Christian Boos
Status: newassigned

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

comment:11 by ebray, 11 years ago

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.

by Christian Boos, 11 years ago

Latest version of the show/hide columns patch

comment:12 by Christian Boos, 11 years ago

Priority: lownormal

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

comment:13 by ebray, 11 years ago

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

comment:14 by Christian Boos, 11 years ago

Keywords: review removed
Resolution: fixed
Status: assignedclosed

Applied in [7674], thanks for the feedback!

comment:15 by ebray, 11 years ago

Resolution: fixed
Status: closedreopened

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

comment:16 by Christian Boos, 11 years ago

Keywords: javascript added

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

comment:17 by ebray, 11 years ago

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

by ebray, 11 years ago

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

comment:18 by ebray, 11 years ago

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.

comment:19 by Christian Boos, 11 years ago

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.

comment:20 by Christian Boos, 11 years ago

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

in reply to:  20 ; comment:21 by ebray, 11 years ago

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.

in reply to:  21 comment:22 by ebray, 11 years ago

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.

comment:23 by Christian Boos, 11 years ago

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.

comment:24 by ebray, 11 years ago

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.

by Christian Boos, 11 years ago

Attachment: t7055-alt-r7933.patch added

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

comment:25 by Christian Boos, 11 years ago

Component: generalrendering

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.

comment:26 by Christian Boos, 10 years ago

The change should also benefit to #5499.

comment:27 by Christian Boos, 10 years ago

Milestone: 0.120.12.1

Not critical for the release.

comment:28 by Ryan Ollos <ryano@…>, 10 years ago

Cc: ryano@… added

comment:29 by Carsten Klein <carsten.klein@…>, 10 years ago

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

comment:30 by Carsten Klein <carsten.klein@…>, 10 years ago

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

comment:31 by Christian Boos, 9 years ago

Keywords: performance added
Milestone: 0.12.1next-major-0.1X

comment:32 by kamil@…, 9 years ago

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.

comment:33 by mike.schall@…, 9 years ago

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

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

comment:35 by Ryan J Ollos, 5 years ago

Cc: Ryan J Ollos added; ryano@… removed

comment:36 by Ryan J Ollos, 4 years ago

Owner: Christian Boos removed
Status: reopenednew

comment:37 by figaro, 4 years ago

Keywords: patch added

comment:38 by walty <walty8@…>, 3 years ago

Cc: walty8@… added

comment:39 by walty <walty8@…>, 3 years ago

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.

comment:40 by Jun Omae, 3 years ago

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.

Last edited 3 years ago by Jun Omae (previous) (diff)

by walty <walty8@…>, 3 years ago

Attachment: inline-after.png added

by walty <walty8@…>, 3 years ago

Attachment: inline-after.2.png added

by walty <walty8@…>, 3 years ago

Attachment: in-line-before.png added

by walty <walty8@…>, 3 years ago

Attachment: side-by-side.png added

comment:41 by walty <walty8@…>, 3 years ago

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:

https://trac.edgewall.org/raw-attachment/ticket/7055/in-line-before.png

And this is the selection effect after the patch:

https://trac.edgewall.org/raw-attachment/ticket/7055/inline-after.png

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

https://trac.edgewall.org/raw-attachment/ticket/7055/side-by-side.png

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

comment:42 by Ryan J Ollos, 3 years ago

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.

comment:43 by Ryan J Ollos, 3 years ago

Milestone: next-major-releasesnext-dev-1.3.x

comment:44 by Christian Boos, 3 years ago

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

in reply to:  44 ; comment:45 by Christian Boos, 3 years ago

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.

Last edited 3 years ago by Christian Boos (previous) (diff)

in reply to:  45 ; comment:46 by Christian Boos, 3 years ago

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.

in reply to:  46 ; comment:47 by Jun Omae, 3 years ago

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
Last edited 3 years ago by Jun Omae (previous) (diff)

in reply to:  47 ; comment:48 by Christian Boos, 3 years ago

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

in reply to:  48 ; comment:49 by Jun Omae, 3 years ago

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.

in reply to:  49 ; comment:50 by walty <walty8@…>, 3 years ago

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.

in reply to:  50 ; comment:51 by Jun Omae, 3 years ago

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.

in reply to:  51 comment:52 by walty <walty8@…>, 3 years ago

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.

in reply to:  44 ; comment:53 by walty <walty8@…>, 3 years ago

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.

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

in reply to:  48 comment:54 by Christian Boos, 3 years ago

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

comment:55 by Ryan J Ollos, 3 years ago

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

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

in reply to:  53 comment:56 by Christian Boos, 3 years ago

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/…)

in reply to:  55 comment:57 by Christian Boos, 3 years ago

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

comment:58 by Ryan J Ollos, 6 weeks ago

Milestone: next-dev-1.3.xnext-dev-1.5.x

Milestone renamed

comment:59 by Ryan J Ollos, 2 weeks ago

Milestone: next-dev-1.5.xnext-major-releases

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned. Next status will be 'new'.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


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