Conversation
| } | ||
| }, | ||
| internalUrl: function() { | ||
| return "/view/" + this.get('tree') + "/" + this.get('path') + "#L" + this.get('lno'); |
There was a problem hiding this comment.
If we have a version hash, we should be embedding it here, so that this link, if copied/pasted, continues to point to the right line later on.
There was a problem hiding this comment.
I think there's a bigger question here around link behaviour--we'll need to figure out some story around when we want to permalink a specific version vs when we just want to link to HEAD. It's easy to change the behaviour later, so I'm most interested right now in making sure the URL format is satisfactory.
There was a problem hiding this comment.
fwiw there's one affordance for handling this right now, which is the -revparse flag to codesearch, which causes it to resolve all commit references into sha1's while indexing, which will then propagate through the system to ensure stable references and links.
I'm a big fan of linking to sha1's in general, but there are definitely a bunch of fiddly UX questions.
web/templates/fileview.html
Outdated
| <li>Shift + click a second line number to highlight a range</li> | ||
| <li>Press <pre class="keyboard-shortcut">/</pre> to start a new search (the selected text will be prefilled)</li> | ||
| <li>Select some text and press <pre class="keyboard-shortcut">enter</pre> to perform a new search with the selected text</li> | ||
| <li>Press <pre class="keyboard-shortcut">d</pre> to visit this file in diffusion</li> |
There was a problem hiding this comment.
"diffusion" is a Phabricator-ism. "...to visit this file elsewhere" (as done above) seems handwavy, though.
There was a problem hiding this comment.
Whoops! I changed it to "elsewhere" above, though there's javascript that rewrites it to mention the specific domain. I suppose that's available at template render time, so we might as well do it then.
I'm reminded that I changed the shortcut to "v" and should update this help text.
This adds the ability to view source files directly in livegrep, as an alternative to linking to external viewers configured by `url-pattern`. This is desirable for a couple of reasons: - It's a common pattern to search for something in the codebase, find it, examine its surroundings, and search again for something nearby (for example, when following a chain of function calls). - The external viewer might be slow (for example, if it's Phabricator), and it's valuable to get a snappy response when clicking through to view a search result. The file browser is implemented as part of `livegrep`, and requires having git repositories available to the `livegrep` process. Therefore, `livegrep` now accepts an additional config file (in the same format used by `codesearch`) specifying where to find repos on disk. If this config is not provided, we retain the old behaviour where clicking through to view a search result goes directly to the external site.
|
Just rebased onto master and resolved merge conflicts. (@nelhage, do you prefer rebasing feature branches onto master or merging master into branches?) |
|
I'm generally in favor of rebasing onto master. I'll try to take a look at this this week. I've been pretty underwater of late. |
3 participants
This adds the ability to view source files directly in livegrep, as
an alternative to linking to external viewers configured by
url-pattern. This is desirable for a couple of reasons:It's a common pattern to search for something in the codebase,
find it, examine its surroundings, and search again for something
nearby (for example, when following a chain of function calls).
The external viewer might be slow (for example, if it's Phabricator),
and it's valuable to get a snappy response when clicking through to
view a search result.
The file browser is implemented as part of
livegrep, and requireshaving git repositories available to the
livegrepprocess. Therefore,livegrepnow accepts an additional config file (in the same formatused by
codesearch) specifying where to find repos on disk. If thisconfig is not provided, we retain the old behaviour where clicking
through to view a search result goes directly to the external site.
Fixes #55.
Credit again to @christoffer for the initial implementation of this in
Dropbox's internal fork; I've made some additional tweaks porting it
here.