[PATCH] Highlight selected line when #L123 is used in source browser

[shesek]

When viewing a file via the source browser and #L<line number> is used, it would be nice if it'll visually show the line that was passed, especially when its near the end so its not scrolled down right to that line. Also, when it does and scroll the page up and down, its nice to still see the original line you were linked to.

[shesek]

Obviously, if you don't like the visual appearance, feel free to change it. There are some really ​cool colors to pick from at colourlovers (an excellent site I use very often for color and palettes inspiration) (had to use tinyurl because it the spam filter wouldn't let it pass otherwise)

[Christian Boos]

We could indeed highlight the referenced line, in some way.

However, the way you did it will interfere with the highlighting done via the marks specified in the TracLinks#source:links (e.g. source:/trunk/trac/versioncontrol/web_ui/browser.py@10123:782-786,801#L779​).

Maybe we could simply add the .hilite class to the targeted line, in case there are no mark set? This would at the same time make the highlighting consistent and not interfere with marks when a range of lines to highlight is already specified.

Now, as a follow-up to this (and as you seem to be very interested in Javascript hacking ;-) ), you may find an idea how to make it easy to define highlighted ranges in the first place. E.g. click on the line <a> does a select of that line and unselects all the others, CTRL toggles the selection, the SHIFT modifier does the same but for a range starting from the last selected line, and then we would modify the location hash accordingly and even create a dynamic "Link: source:path@rev:range" text somewhere for easy copying…

Currently, the mark feature (#2764), as nice as it is, is underused as it's quite painful to build the link manually…

[shesek]

Those are some really cool ideas! I currently have a bit of a work load, I really want to continue with #9643, BUT when I get some spare time I'll get to work on that

[shesek]

Okay, so I did manage to find some time between all the client work :-) got it to work as you suggested. Tested on Chrome 6, FF 3.6.10, Safari 4, Opera 10, IE8 and IE7.

It has a few minor issues:

• I didn't quite find where the put the source: link, currently its right above the first line of code. Can someone find a better place to put that at?
• In some browsers, when shift/ctrl is used, I couldn't suppress the default behavior (selecting text/table columns) without disabling selecting text completely. Anyone know of a way?
• In some browsers there's an annoying 1/5 a second delay when selecting lines. I don't quite know what causes it, but I suspect that changing the hash is causing some browsers to do something odd.

Also, I changed the original code (to highlight #L123) to use the .hilite class, but I still think my yellow color looks better than the purple one .hilite uses :-)

[Christian Boos]

That's really great! I like it a lot.

• about the source:... link, we could just append the range to the existing source: ... / ... @... <h1> heading
• I noticed while debugging the text range gets selected before we even enter our handler, so there's no point in trying to stop the propagation of the event, it's too late. Maybe we could just try to unselect the text afterwards? Another idea: disable text selection in the keypress event when Shift is pressed?
• I only noticed the slow down in IE? Could that performance issue be due to the fact that a lot of elements (all lines) need to be bound? Maybe #7055 and using <pre> could help here…
• the modification of the URL is wrong, as you change it to something like ...?rev=123#marks=1075,1144-1161; it should be ...?rev=123&marks=1075,1144-1161 and even ...?rev=123&marks=1075,1144-1161#1075 if there's no hash already

As for the highlight color, I don't remember the rationale for the original color (maybe there wasn't any ;-) ), so I'm OK for something yellow, though I would prefer something slightly less "shiny". What about simply #FFA which we already use on the timeline?

Last suggestion: there seem to be no way to deselect a range? Maybe when we're about to add a range for which all the lines are already part of the selection, this range could be deselected?

[shesek]

Replying to cboos:

That's really great! I like it a lot.

• about the source:... link, we could just append the range to the existing source: ... / ... @... <h1> heading

Oh, cool. I didn't think of that option, I'll change that

• I noticed while debugging the text range gets selected before we even enter our handler, so there's no point in trying to stop the propagation of the event, it's too late. Maybe we could just try to unselect the text afterwards? Another idea: disable text selection in the keypress event when Shift is pressed?

Your first suggestion seems to work well - instead of preventing text from being selected, I can un-selecte it afterwards using window.getSelection ? window.getSelection().removeAllRanges() : document.selection.empty().

I can do that whenever a click is made and Shift is pressed, but I'm not quite sure how to do that without annoying the user - what if someone uses Shift because he really does want to select a text range?

• I only noticed the slow down in IE? Could that performance issue be due to the fact that a lot of elements (all lines) need to be bound? Maybe #7055 and using <pre> could help here…

I've had code in the past that made much more complex DOM manipulation and haven't had those issues, so I'm not quite sure what could cause it :S

• the modification of the URL is wrong, as you change it to something like ...?rev=123#marks=1075,1144-1161; it should be ...?rev=123&marks=1075,1144-1161 and even ...?rev=123&marks=1075,1144-1161#1075 if there's no hash already

I'm not quite sure I understood what you meant, but if you want it to put the lines in the query string instead the hash - I can't change the it without causing a full page refresh, its only possible to modify the hash portion of the URL. When a URL with the hash is used, my code makes sure to highlight those lines.

As for the highlight color, I don't remember the rationale for the original color (maybe there wasn't any ;-) ), so I'm OK for something yellow, though I would prefer something slightly less "shiny". What about simply #FFA which we already use on the timeline?

#FFA looks great. Should I change .hilite background-color in the next patch I'm uploading?

Last suggestion: there seem to be no way to deselect a range? Maybe when we're about to add a range for which all the lines are already part of the selection, this range could be deselected?

I tried selecting ranges in Windows and and replicate the behavior - there wasn't a way to un-select a range, so I'm not quite sure if its needed. Checking first if all the range is already selected will also add some more complexity. Should I add it?

Thanks for the feedback and suggestions!

[shesek]

And another thing - selecting lines range doesn't seem to work in blame view. Is that even needed there? is there a source: link that can link to the blame view with line marks?

[Christian Boos]

Replying to shesek:

… Your first suggestion seems to work well - instead of preventing text from being selected, I can un-selecte it afterwards using window.getSelection ? window.getSelection().removeAllRanges() : document.selection.empty().

I can do that whenever a click is made and Shift is pressed, but I'm not quite sure how to do that without annoying the user - what if someone uses Shift because he really does want to select a text range?

I'm afraid it's incompatible behavior, either select a range of lines or a range of text, but we probably can't guess which of these two the user wanted to do, unless we restrict ourselves to the line number column for doing the line selection.

• I only noticed the slow down in IE? Could that performance issue be due to the fact that a lot of elements (all lines) need to be bound? Maybe #7055 and using <pre> could help here…

I've had code in the past that made much more complex DOM manipulation and haven't had those issues, so I'm not quite sure what could cause it :S

We can still optimize later.

• the modification of the URL is wrong, as you change it to something like ...?rev=123#marks=1075,1144-1161; it should be ...?rev=123&marks=1075,1144-1161 and even ...?rev=123&marks=1075,1144-1161#1075 if there's no hash already

I'm not quite sure I understood what you meant, but if you want it to put the lines in the query string instead the hash -

I wanted to have a meaningful URL which could work even without Javascript, but …

I can't change the it without causing a full page refresh, its only possible to modify the hash portion of the URL.

… forgot about that. So indeed your solution makes sense (maybe we could even remove marks= to get prettier URLs?).

When a URL with the hash is used, my code makes sure to highlight those lines.

#FFA looks great. Should I change .hilite background-color in the next patch I'm uploading?

Yep.

Last suggestion: there seem to be no way to deselect a range? Maybe when we're about to add a range for which all the lines are already part of the selection, this range could be deselected?

I tried selecting ranges in Windows and and replicate the behavior - there wasn't a way to un-select a range

Ok, so In Windows 7's explorer at least, as long as you don't change the "anchor" point by doing another single click (or Ctrl click), selecting an entry with Shift+click changes the last range, so giving you a (limited) way to do the unselection. I think that would be fine. Still in Windows, if you don't do Ctrl in addition to Shift, that last range replaces what has been selected so far… but I prefer the way you did it, ranges can only be added, so you don't risk to lose a sequence of selections by inadvertently missing the Ctrl key. However, having said that, I now realize that another single click will also make you lose the selection. That can also be annoying when you first go to a source page following a source: link with marks. It's perhaps still too easy to lose the selection, and this could be yet another argument in favor on limiting this interaction to the line number column.

[shesek]

Replying to cboos:

Replying to shesek: I'm afraid it's incompatible behavior, either select a range of lines or a range of text, but we probably can't guess which of these two the user wanted to do, unless we restrict ourselves to the line number column for doing the line selection.

Yes, I was thinking about that option too. In fact, that's how it worked at the beginning, than I changed it to the entire line - but wasn't 100% sure about that chance.

maybe we could even remove marks= to get prettier URLs?

We can, but I did with #marks= to keep it similar to ?marks=, so it'll visually look about the same and to make it easy to replace the # with an ? or & and transform it into a URL that doesn't require JavaScript.

I also need some way to identify when its line marks and not some other anchor, but I can also do that with a shorter string (#M19,25-30) or by simply checking if the hash begins with a number (#19,25-30).

Ok, so In Windows 7's explorer at least, …., selecting an entry with Shift+click changes the last range

I also did my testing with explorer. When selecting with shift (without ctrl), it selects only items between the 'anchor point' (the item with the dotted border) and the item you clicked, while removing everything else. Its not possible, for example, to select one item using ctrl and than adding another range to it. When I did use ctrl, it always appended the new range to the previous selections. So I'm not quite sure what you mean by 'changes the last range' - for me, it either changes everything or always appending to previous selections.

I now realize that another single click will also make you lose the selection… and this could be yet another argument in favor on limiting this interaction to the line number column.

I think so too, but not 100% sure. It'll make the clickable area much smaller and less convenient to use… but unless we can think of another solution for cases where you're already coming from a source: link and just click somewhere, I guess we'll have to do it that way.

Also, what about the blame view? I think support is needed there too - but is there any way to link to it with marks in a similar way to the source: link?

[Remy Blank]

Preparing for 1.0.

[Christian Boos]

Move feature requests to next-dev.

[Christian Boos]

well, once again… next-dev

[Ryan J Ollos]

Retargetting tickets to narrow focus for milestone:1.2. Please move the ticket back to milestone:next-dev-1.1.x if you intend to resolve it by milestone:1.2.