Edgewall Software
Modify

Opened 8 years ago

Closed 8 years ago

Last modified 2 years ago

#10284 closed enhancement (fixed)

[PATCH] Fragment identifier search

Reported by: psuter <petsuter@…> Owned by: Peter Suter
Priority: normal Milestone: 1.0
Component: general Version:
Severity: normal Keywords: search
Cc: Branch:
Release Notes:

TracLinks may now link to a specific word within a source file or attachment using a URL fragment such as #/word (requires Javascript)

API Changes:

Description

As discussed on TracDev/PluginDevelopment, a way to deep link into pages a bit more robust to changes than specifying an anchor like #L123 would be useful.

I propose a JavaScript approach that dynamically searches and scrolls to a term specified in the fragment identifier.

cboos proposed a syntax (based on a vim-ism?) for scrolling to the first match using #/term and to the last match using #?term.

For example the link tags/trac-0.5/setup.py#/import would link to line 3 (import sys), while tags/trac-0.5/setup.py#?import would link to line 8.

The attached patch implements this by extending the existing search term highlighting. (Note that this makes it available on all searchable pages, like in the source file browser or in the wiki.)

The exact syntax could be changed or extended. Maybe #term:n for the nth and #term:-n for the nth last match?

Attachments (2)

hash-search.patch (1.8 KB ) - added by psuter <petsuter@…> 8 years ago.
Implements #/forward and #?backward fragment identifier search
hash-search-case-sensitive.patch (2.6 KB ) - added by Christian Boos 8 years ago.
applies on top of hash-search.patch

Download all attachments as: .zip

Change History (22)

by psuter <petsuter@…>, 8 years ago

Attachment: hash-search.patch added

Implements #/forward and #?backward fragment identifier search

comment:1 by Peter Suter, 8 years ago

Milestone: 0.13
Owner: set to Peter Suter

comment:2 by Christian Boos, 8 years ago

I've played a bit with the patch. It worked great though at first I expected the search to be case sensitive, and it was not. I think that in this situation, it's better if the search is case sensitive by default. For example when creating "search anchors", we would certainly always respect the case of a function we want to search for, or respect a capitalized title. And we could add the /i suffix (another vim-ism) if one really would like to have case insensitive search.

Some nitpicks about style: I know our TracDev/CodingStyle doesn't mention Javascript code, but nevertheless it seems we most of the time try to use lower case identifiers for variable names and camelCase only for functions. Btw, I think non anonymous function should be defined as function fooBar() {...} rather than var fooBar = function() {...}.

I'm adding a new version of the patch which adds support for the /i suffix and updates the style (except in the first function, as the style there is currently different, because that function was copied from somewhere else).

by Christian Boos, 8 years ago

applies on top of hash-search.patch

comment:3 by Christian Boos, 8 years ago

(there's a small glitch with my change which I just detected; it is apparent in the normal search results as the highlighted terms are turned into lower case…)

in reply to:  2 ; comment:4 by Peter Suter, 8 years ago

Thanks for the feedback.

Replying to cboos:

I think that in this situation, it's better if the search is case sensitive by default.

I agree, but didn't want to make the initial patch too complicated.

Some nitpicks about style: … I know our TracDev/CodingStyle doesn't mention Javascript code

Noted. Maybe we can add a brief summary. Or point to an existing style guide. (There seem to be almost infinitely many JavaScript style guides, but none seem to match the Trac conventions too closely as far as I can tell.

in reply to:  4 ; comment:5 by Christian Boos, 8 years ago

Replying to psuter:

.. JavaScript

The style guide from Douglas Crockford seems to be a pretty close match, though. We could add a few local adaptations on that basis. Also, it advises 4 spaces indentation unit, and I would love to switch our code to that convention!

Besides, we could start to document a bit more our Javascript code, there are lots of .js files with just nothing, not even a © Edgewall line…

I'm not sure if the lack of comments and the 2 spaces indentation have a common root in trying to keep those files as small as possible, but if that's the case, we should rather achieve this goal with a minification step (put all our js code in a trac_min.js file, with a make minify step).

style

Funny, the Function Declarations Within Blocks item justifies using var fooBar = function() {...}. But since the jQuery source code doesn't follow this rule, I think the caveats listed there must be of little practical concern, and I think we can indeed use the function fooBar() {...} style if we agree it's nicer that way.

in reply to:  5 comment:6 by Peter Suter, 8 years ago

Replying to cboos:

The style guide from Douglas Crockford seems to be a pretty close match, though. We could add a few local adaptations on that basis.

I added some of your and Remy's advice (from #10270) to TracDev/CodingStyle as a first step to that. Expand / expunge as you see fit. :)

we should rather achieve this goal with a minification step (put all our js code in a trac_min.js file, with a make minify step).

The minification topic seems to come up frequently (e.g. in #10245). Instead of a static pre-process, would a dynamic (cached?) processing of scripts passed to add_script be feasible?

in reply to:  3 comment:7 by Peter Suter, 8 years ago

Replying to cboos:

(there's a small glitch with my change which I just detected; it is apparent in the normal search results as the highlighted terms are turned into lower case…)

That should be easily fixable:

  • trac/htdocs/js/search.js

    diff -r 09e933afa83c -r b9507aaf2f5c trac/htdocs/js/search.js
    a b  
    55    function highlight(node) {
    66      if (node.nodeType == 3) { // Node.TEXT_NODE
    77        var val = node.nodeValue;
    8         if (!caseSensitive)
    9           val = val.toLowerCase();
    10         var pos = val.indexOf(text);
     8        var pos = (caseSensitive ? val : val.toLowerCase()).indexOf(text);
    119        if (pos >= 0 && !$(node.parentNode).hasClass(className)) {
    1210          var span = document.createElement("span");
    1311          span.className = className;

(Or using a new variable for the possibly-lowercase-val, if you dislike the ternary operator.)

Should I commit it like this, or would you wait for more opinions?

comment:8 by Remy Blank, 8 years ago

+1 from my side for applying.

comment:9 by Christian Boos, 8 years ago

Final result is fine for me as well (and FWIW, I like using the ternary operator as well; on trunk we even use it for Python with its A if COND else B equivalent).

If you wish at a later point to add the :n extension, it's also fine for me. But what would eventually be even more useful (in the perspective of providing robust links) is the possibility of using a regexp for the search term.

comment:10 by Peter Suter, 8 years ago

Resolution: fixed
Status: newclosed

Applied in [10774]. I left the more advanced extensions out for now. Let's see if they'll actually be missed.

Should this functionality be documented somewhere? The closest existing wiki content I could find are TracLinks#Relativelinks and WikiFormatting#SettingAnchors but it doesn't really seem to fit on either of those pages.

in reply to:  10 ; comment:11 by Remy Blank, 8 years ago

Replying to psuter:

Applied in [10774].

Excellent!

Should this functionality be documented somewhere?

Yes, it should. I would add a new section to the "Advanced use of TracLinks" section.

in reply to:  11 ; comment:12 by Peter Suter, 8 years ago

Replying to rblank:

Yes, it should. I would add a new section to the "Advanced use of TracLinks" section.

Should I add it to that (0.12) page you linked, or modify trunk/trac/wiki/default-pages/TracLinks, or how is this usually done?

I'd merge the first part of Relative Links about anchors with something like this:

=== Link anchors ===

To create a link to a specific anchor in a page, use '#':
{{{
 [#Relativelinks relative links] or [[#Relativelinks|relative links]]
}}}
Displays:
  [#Relativelinks relative links] or [[#Relativelinks|relative links]]

Hint: when you move your mouse over the title of a section, a '¶' character will be displayed. This is a link to that specific section and you can use this to copy the `#...` part inside a relative link to an anchor.

To create a link to the first or last occurrence of a term on a page, use a ''pseudo anchor'' starting with '#/' or '#?':
{{{
 [#/Milestone first occurrence of Milestone] or [#?Milestone last occurrence of Milestone]
}}}
Displays:
 [#/Milestone first occurrence of Milestone] or [#?Milestone last occurrence of Milestone]
This will also highlight all other matches on the linked page. By default only case sensitive matches are considered. To include case insensitive matches append '/i':
{{{
 [#/Milestone/i first occurrence of Milestone or milestone] or [#?Milestone/i last occurrence of Milestone or milestone]
}}}
Displays:
 [#/Milestone/i first occurrence of Milestone or milestone] or [#?Milestone/i last occurrence of Milestone or milestone]

''(since Trac 0.13)''

in reply to:  12 ; comment:13 by Remy Blank, 8 years ago

Replying to psuter:

Should I add it to that (0.12) page you linked, or modify trunk/trac/wiki/default-pages/TracLinks, or how is this usually done?

You should edit the wiki here on t.e.o. The pages are synced regularly to trunk with a script.

I'd merge the first part of Relative Links about anchors with something like this:

Yes, that looks nice. I notice that the section only shows links to anchors on the page containing the link. Maybe you could also add a few examples of links to anchors on other pages? For example a link to a specific interface in a file in the source browser.

in reply to:  13 ; comment:14 by Peter Suter, 8 years ago

Replying to rblank:

You should edit the wiki here on t.e.o. The pages are synced regularly to trunk with a script.

Neat. I added some examples to TracLinks#Linkanchors.

in reply to:  14 ; comment:15 by Christian Boos, 8 years ago

Replying to psuter:

Replying to rblank:

You should edit the wiki here on t.e.o. The pages are synced regularly to trunk with a script.

Neat. I added some examples to TracLinks#Linkanchors.

Actually this should have been 0.13/TracLinks, as the default page is reserved for documenting the default stable, at least until we're close enough to the release of the next version.

In more details, the workflow for the TracGuide pages goes like this:

  • "most of the time"
    • changes for current stable version X are documented in toplevel wiki pages
    • changes on trunk (version X+1) are documented in (X+1)/…
  • when approaching the release (usually at the time of the beta)
    • toplevel pages are copied to X/… as a snapshot
    • (X+1)/… pages are copied to toplevel
    • all toplevel pages are reviewed and updated as needed for the new version X

(I usually take care of the copying / moving around - maybe I'll even write a script this time!)

comment:16 by Remy Blank, 8 years ago

Sorry, my bad advice. Even I wasn't exactly clear how this worked. Maybe you could copy the workflow above into a sub-page of TracDev (if it's not already there, that is)?

in reply to:  15 ; comment:17 by Peter Suter, 8 years ago

Replying to cboos:

Actually this should have been 0.13/TracLinks, as the default page is reserved for documenting the default stable, at least until we're close enough to the release of the next version.

I deleted my previous change (documenting this ticket) from the toplevel page, copied it to 0.13/TracLinks and added my change back there: 0.13/TracLinks#Linkanchors

I hope I got it right this time.

in reply to:  17 comment:18 by Christian Boos, 8 years ago

Replying to psuter:

I hope I got it right this time.

Perfect! Thanks.

comment:19 by Alex Willmer <al.willmer@…>, 8 years ago

Release Notes: modified (diff)

comment:20 by Ryan J Ollos, 2 years ago

See also #12756.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Peter Suter.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


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