search

astoff / devdocs.el

Emacs viewer for DevDocs
289 stars 17 forks source link

Respect browse-url-handlers #26

Closed bon closed 1 year ago

bon commented 1 year ago

If the user had set browse-url-handlers, it would have overridden browse-url-browser-function. This fixes it.

astoff commented 1 year ago

The right thing to do here would be to set browse-url-handlers and not modify the fallback browse-url-browser-function, but this seems tricky to do while keeping compatibility with Emacs 27.

astoff commented 1 year ago

Actually, I'm not sure now why the current code would be incompatible with Emacs 28's browse-url-handlers. If the URL contains a colon, handling should fall back to the default mechanism.

Which settings do you have that lead to problems?

bon commented 1 year ago

The right thing to do here would be to set browse-url-handlers and not modify the fallback browse-url-browser-function, but this seems tricky to do while keeping compatibility with Emacs 27.

Not sure what you mean. The pull request does not modify browse-url-browser-function

bon commented 1 year ago

Actually, I'm not sure now why the current code would be incompatible with Emacs 28's browse-url-handlers. If the URL contains a colon, handling should fall back to the default mechanism.

Which settings do you have that lead to problems?

The point is that if the user has defined browse-url-handlers, with or withoud colons, it won't be picked up by devdocs.el

astoff commented 1 year ago

Not sure what you mean. The pull request does not modify browse-url-browser-function

Yes, I mean that the current code, written for Emacs 27, hacks into browse-url-browser-function. This is ugly and becomes unnecessary in the new API of Emacs 28, but we still promise to support Emacs 27, and it's a bit too early to drop it.

The point is that if the user has defined browse-url-handlers, with or withoud colons, it won't be picked up by devdocs.el

Right, the only issue currently is that our handler has lower precedence over the user settings. But the user shouldn't write a handler that matches just about any URL. For instance, the URLs that devdocs.el should handle are never of the form scheme:.... On the other hand, if the user whats a special handling of certain external URLs (which do appear in devdocs), we shouldn't just override that.

So I wonder, what kind of conflict have you experienced?

bon commented 1 year ago

Not sure what you mean. The pull request does not modify browse-url-browser-function

Yes, I mean that the current code, written for Emacs 27, hacks into browse-url-browser-function. This is ugly and becomes unnecessary in the new API of Emacs 28, but we still promise to support Emacs 27, and it's a bit too early to drop it.

I see what you mean. So it could select on version?

The point is that if the user has defined browse-url-handlers, with or withoud colons, it won't be picked up by devdocs.el

Right, the only issue currently is that our handler has lower precedence over the user settings. But the user shouldn't write a handler that matches just about any URL. For instance, the URLs that devdocs.el should handle are never of the form scheme:.... On the other hand, if the user whats a special handling of certain external URLs (which do appear in devdocs), we shouldn't just override that.

So I wonder, what kind of conflict have you experienced?

I have browse-url-function set to use eww-browse-function for a bunch of regexps, mostly documentation. The default goes to Firefox. Without this pull request this was failing for devdocs.

astoff commented 1 year ago

As far as I know EWW can only handle HTTP links, so your matchers should probably start with "\\`https?://".

So while I could improve the url handling (which I'll try and do when I have time), I don't think this is a serious bug.

bon commented 1 year ago

As far as I know EWW can only handle HTTP links, so your matchers should probably start with "\\`https?://".

This is not true. It works for https://devdocs.io/.* so I can read devdocs without leaving emacs.

So while I could improve the url handling (which I'll try and do when I have time), I don't think this is a serious bug.

Your project, your call.

astoff commented 1 year ago

Wait, https://devdocs.io/.* is an HTTP URL, so it is true that your browse-url-handlers matcher should start with "\\`https?://".

bon commented 1 year ago

Wait, https://devdocs.io/.* is an HTTP URL, so it is true that your browse-url-handlers matcher should start with "\\`https?://".

Correct. I view devdocs in eww in emacs

astoff commented 1 year ago

I've pushed a fix that should work in all cases.