Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'C-u C-c C-o' to open links with find-file #132

Closed

Conversation

wking
Copy link

@wking wking commented Apr 27, 2016

I edit a lot of Markdown that cross-links local files. This commit
adds the ability to follow those links with find-file, instead of
opening them in a browser. The fragment-removal in
markdown-link-filename supports links like:

See [the README](README.md#license).

to work.

It would be nice if the find-file logic used the fragment in such
links to scroll to the appropriate anchor in the target file. But
often that target is autogenerated from a slugged header, and that
seemed to magical to be worth figuring out.

It would be nice if the find-file vs. browse-url logic happened
automatically, but I'm not fluent enough in Emacs Lisp for that to be
worth the trouble.

Apologies if this patch is terrible ;). Emacs Lisp is very foreign to
me, so what I have here may be wrong or not idiomatic. Hopefully it's
close enough for you to understand where I'm trying to go :p. I'm
happy to reroll if anyone has suggestions.

@syohex
Copy link
Collaborator

syohex commented Apr 27, 2016

Is it enough to use M-x ffap(find-file-at-point) in such case ?

@wking
Copy link
Author

wking commented Apr 27, 2016

On Tue, Apr 26, 2016 at 11:12:12PM -0700, Syohei YOSHIDA wrote:

Is it enough to use M-x ffap(find-file-at-point) in such case ?

Not if:

  1. You're on the link text and not the link target (although there is
    always markdown-jump).
  2. The link target does not contain fragments (in which case ffap
    seems to prompt you for a file in the target directory).

So ffap is better than nothing, but I think something like this patch
is better than ffap.

(defun markdown-follow-link-at-point (&optional arg)
"Open the current non-wiki link in a browser.
With prefix argument ARG, open the file with `find-file'.
See `markdown-link-p' and `markdown-follow-link'."
(interactive)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please specify "P" for users who use this command directly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Apr 27, 2016 at 12:11:18AM -0700, Syohei YOSHIDA wrote:

+(defun markdown-follow-link-at-point (&optional arg)

  • "Open the current non-wiki link in a browser.
    +With prefix argument ARG, open the file with find-file'. +Seemarkdown-link-p' and `markdown-follow-link'."
    (interactive)

Please specify "P" for users who use this command directly.

Thanks, done with e1e24e2bdaf964.

@wking wking force-pushed the follow-reference-link-with-find-file branch from e1e24e2 to bdaf964 Compare April 27, 2016 07:14
I edit a lot of Markdown that cross-links local files. This commit
adds the ability to follow those links with find-file, instead of
opening them in a browser. The fragment-removal in
markdown-link-filename supports links like:

  See [the README](README.md#license).

to work.

It would be nice if the find-file logic used the fragment in such
links to scroll to the appropriate anchor in the target file.  But
often that target is autogenerated from a slugged header, and that
seemed to magical to be worth figuring out.

It would be nice if the find-file vs. browse-url logic happened
automatically, but I'm not fluent enough in Emacs Lisp for that to be
worth the trouble.
(defun markdown-follow-link (name &optional other)
"Follow the link NAME in a browser.
Open the with `find-file' when OTHER is non-nil."
(if other (find-file (markdown-link-filename name))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is ffap better rather than find-file ? If the link is HTTP URL(like http://github.com/docker/docker/blob/master/README.md), C-u M-x markdown-follow-link-at-point creates terrible file.

And should file open function be customizable ? (For example default value is find-file or ffap and user sets find-file-other-window or their own function).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Apr 27, 2016 at 12:47:18AM -0700, Syohei YOSHIDA wrote:

+(defun markdown-follow-link (name &optional other)

  • "Follow the link NAME in a browser.
    +Open the with `find-file' when OTHER is non-nil."
  • (if other (find-file (markdown-link-filename name))

Is ffap better rather than find-file ? If the link is HTTP
URL(like http://github.com/docker/docker/blob/master/README.md),
C-u M-x markdown-follow-link-at-point creates terrible file.

Ah, that sounds like the “find-file vs. browse-url logic” I wished for
in the initial comment 1. However, in this case the target is a
name, not at the point. Ideally we'd have a
find-file-with-ffap-url-regexp. Does a function like that exist?
Should we use ffap-url-regexp ourselves?

With the current patch, I'm leaving it up to the user to C-u as
appropriate, but with reference-style links, they may not have the
link-target available to inform their choice.

@jrblevin
Copy link
Owner

jrblevin commented May 6, 2016

I implemented something different in commit 8e708bc, which I think accomplishes the automatic selection you wanted. Please try it out. It does not require changing or prefixing the keybinding you use. You can still open links with C-c C-o. Fully specified URLs will be opened in a browser, as usual, while other "links" will be opened using find-file after stripping query strings and anchors. I think this will solve your problem. The existing approach—opening local links, query strings and all, with browse-url—was flawed anyhow.

@wking
Copy link
Author

wking commented May 6, 2016

On Fri, May 06, 2016 at 09:52:51AM -0700, Jason Blevins wrote:

I implemented something different in commit 8e708bc, which I think
accomplishes the automatic selection you wanted.

A problem with both our approaches is handling intra-file links
(e.g. top). Testing 27067e2 on such links is giving me:

find-file-noselect: Wrong type argument: stringp, nil

we probably just want to check for that case and make the follow a
no-op.

Other than that, 27067e2 seems to work for me.

@jrblevin
Copy link
Owner

jrblevin commented May 6, 2016

Thanks--I think this is fixed now in 619d813. Please let me know otherwise.

@wking wking closed this May 6, 2016
@wking
Copy link
Author

wking commented May 6, 2016

On Fri, May 06, 2016 at 11:27:43AM -0700, Jason Blevins wrote:

Thanks--I think this is fixed now in 619d813. Please let me know
otherwise.

Works for me, thanks :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants