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

feat(commands): open urls with goto_file command #5820

Merged
merged 5 commits into from
Nov 21, 2023

Conversation

matoous
Copy link
Contributor

@matoous matoous commented Feb 4, 2023

Add capability for goto_file command to open an URL under cursor.

This pulls in additional dependency - open by @Byron which seems to be very minimal and as such doesn't impact the compile time and/or the final size of the binary.

The updated implementation of goto_file first attempts to parse the WORD under the cursor as an URL. If it suceeds to do so it tries to open it in a browser (platform specific implementation from the open crate). If we fail to parse the WORD as URL the implementation tries to open it as a file in the editor.

Fixes: #1472
Superseds: #4398

@matoous matoous force-pushed the md/1472-open-url branch 2 times, most recently from d28a23e to 078e059 Compare February 4, 2023 10:22
@Byron
Copy link
Contributor

Byron commented Feb 4, 2023

Thanks for using open!
I think it's worth pointing out that open::that() can actually block (and wait for the child process to finish) depending on the launcher program it ends up using. Thus it's platform dependent and not always guaranteed that it's non-blocking - typically on linux with xdg-open.

The opener crate seems to handle it by not checking the exit code of the launcher at all. It's unclear to me how to properly solve this except to try to detect blocking behaviour. Maybe thanks to helix the open crate can make up its mind on how to solve this and become a worthy dependency that way :). Your feedback of very welcome.

@pascalkuthe
Copy link
Member

pascalkuthe commented Feb 4, 2023

Thanks for using open! I think it's worth pointing out that open::that() can actually block (and wait for the child process to finish) depending on the launcher program it ends up using. Thus it's platform dependent and not always guaranteed that it's non-blocking - typically on linux with xdg-open.

The opener crate seems to handle it by not checking the exit code of the launcher at all. It's unclear to me how to properly solve this except to try to detect blocking behaviour. Maybe thanks to helix the open crate can make up its mind on how to solve this and become a worthy dependency that way :). Your feedback of very welcome.

For helix we don't actually need to wait for open to finish or report a status as open doesn't impact what we do in helix itself. We could just run open as an async job but the problem is that open::that is not async so we would create a blocking future, which is not great with tokio (and doesn't work with our jobs queue as it requires futures with a waker which is not the case for something block like this, we could use spawn_blocking with a channel but that is not exactly ideal).

The best way to deal with blocking here for helix would be to use the tokio Command replacement which allows us to wait for the command status asynchronously.
The future could be dispatched using helixes jobs mechanism. Once the future completes we would set a message like successfully openend ... or opening ... failed.

The easiest way to implement this would be if open simply offered a function which only constructs the Command but doesn't actually execute it. (tokio commands can be constructed from std commands).

Looking at the code there are potentially multiple Commands being executed so this function would need to return a list.
This potentially requires different handeling on windows as you seem to be using ShellExecuteW instead of the std Command mechanisim. Why is that necessary @Byron?

@Byron
Copy link
Contributor

Byron commented Feb 4, 2023

The best way to deal with blocking here for helix would be to use the tokio Command replacement which allows us to wait for the command status asynchronously.
The future could be dispatched using helixes jobs mechanism. Once the future completes we would set a message like successfully openend ... or opening ... failed.

I really like that idea - that would conveniently bypass the blocking issue (in every direction). Even if a launcher blocks, that wouldn't be able to block helix.

Returning a list of Command instances should be fine as well.

On windows, somebody tried to remove the ShellExecuteW already, which itself was contributed for some reason maybe related to terminal windows not popping up, and had to revert it later unfortunately without saying what the actual problem is.

It's probably safe to assume that on windows, this new interface we are talking about can't exist, and one would have to work around it in helix somehow. And if that workaround exists, one might as well apply it for all blocking open::that() calls. I hope I am missing something.

@pascalkuthe
Copy link
Member

The best way to deal with blocking here for helix would be to use the tokio Command replacement which allows us to wait for the command status asynchronously.
The future could be dispatched using helixes jobs mechanism. Once the future completes we would set a message like successfully openend ... or opening ... failed.

I really like that idea - that would conveniently bypass the blocking issue (in every direction). Even if a launcher blocks, that wouldn't be able to block helix.

Returning a list of Command instances should be fine as well.

On windows, somebody tried to remove the ShellExecuteW already, which itself was contributed for some reason maybe related to terminal windows not popping up, and had to revert it later unfortunately without saying what the actual problem is.

It's probably safe to assume that on windows, this new interface we are talking about can't exist, and one would have to work around it in helix somehow. And if that workaround exists, one might as well apply it for all blocking open::that() calls. I hope I am missing something.

I looked into this quite a bit more and it seems that the user simply used the wrong program. He called explorer whereas the right command would be cmd /c start (you can't call start directly, hence going trough cmd).

start internally calls the exact same winodws API: see https://en.wikipedia.org/wiki/Start_(command).
Other editors also seem to be using the same thing. For example a very popular filetree for nvim uses this: preservim/nerdtree#1130

So that refactor should be possible after all.

@Byron
Copy link
Contributor

Byron commented Feb 4, 2023

Thanks for the research! I think cmd /c start is also something that was implemented at some point, it seems oddly familiar. I think it's worth validating the 'terminal windows popup' behaviour before changing it, it should be easy enough to figure out. Right now it definitely doesn't show an extra terminal window.
It's definitely encouraging to see that nerdtree also uses this scheme and it seems fine.

Would @matoous have some time for the open crate refactoring? It's probably two pieces, first to use Command in windows as well, and secondly to expose open::that_commands(…) to provide the commands it would execute in order, instead of executing them, so they can be turned into tokio compatible async processes.

@pascalkuthe
Copy link
Member

pascalkuthe commented Feb 4, 2023

Thanks for the research! I think cmd /c start is also something that was implemented at some point, it seems oddly familiar. I think it's worth validating the 'terminal windows popup' behaviour before changing it, it should be easy enough to figure out. Right now it definitely doesn't show an extra terminal window. It's definitely encouraging to see that nerdtree also uses this scheme and it seems fine.

Would @matoous have some time for the open crate refactoring? It's probably two pieces, first to use Command in windows as well, and secondly to expose open::that_commands(…) to provide the commands it would execute in order, instead of executing them, so they can be turned into tokio compatible async processes.

Yeah it does seem that this approach was used in the past but removed: Byron/open-rs#17

It sounds like there weren't really any issues with this tough just "performance" which I don't think matters much when opening a URL compared to being able to do it correctly asynronously. If you are really attachted to using the winapi call here we could have seperate that_commands variants in the varios os specific modules. For windows the that function would remain as is and the that_commands function would construct Command. For all other platforms the that function could just use the that_commands function internally.

It seems like the bug that this PR reffered to which caused rustup to switch to this function has been fixed in windows 10 since 2017: rust-lang/rustup#499 (comment). It sounds like it only ever occured on a preview build to begin with. From the rustup PR it sounds like this is really not that critical and they did it more as a nice to have: rust-lang/rustup#1117 (comment) (" It also should fix opening docs for the few people still using a year-old insider preview build.")

@Byron
Copy link
Contributor

Byron commented Feb 4, 2023

Great detective work, thank you! Who would have thought that open pulls in the windows crate on windows just to support a preview build.

It definitely sounds like it's best to drop using this function and use Command instead, to allow a unified integration with tokio and less dependencies overall.

To me the path forward seems to be clear and I am happy that thanks to helix the venerable open crate gets to be even more useful and future-proof.

@pascalkuthe pascalkuthe added E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 4, 2023
matoous added a commit to matoous/open-rs that referenced this pull request Feb 5, 2023
Use std `Command` instead of `ShellExecuteW` from windows sys crate.

This change was already attempted in: Byron#25
and later reverted in: Byron#27
and it it seems that it didn't work due to incorrect usage of
`explorer` instead of `cmd /c start`.
(see helix-editor/helix#5820 (comment)
for detailed explanation).

Related: helix-editor/helix#5820
matoous added a commit to matoous/open-rs that referenced this pull request Feb 5, 2023
Adds new `command` function that returns the underlying `std::Command`
that would be called to open given path.

Related: Byron#63
Related: helix-editor/helix#5820
matoous added a commit to matoous/open-rs that referenced this pull request Feb 5, 2023
Adds new `command` function that returns the underlying `std::Command`
that would be called to open given path.

Related: Byron#63
Related: helix-editor/helix#5820
matoous added a commit to matoous/open-rs that referenced this pull request Feb 5, 2023
Adds new `command` function that returns the underlying `std::Command`
that would be called to open given path.

Related: Byron#63
Related: helix-editor/helix#5820
matoous added a commit to matoous/open-rs that referenced this pull request Feb 5, 2023
Adds new `command` function that returns the underlying `std::Command`
that would be called to open given path.

Related: Byron#63
Related: helix-editor/helix#5820
matoous added a commit to matoous/open-rs that referenced this pull request Feb 5, 2023
Adds new `command` function that returns the underlying `std::Command`
that would be called to open given path.

Related: Byron#63
Related: helix-editor/helix#5820
matoous added a commit to matoous/open-rs that referenced this pull request Feb 5, 2023
Adds new `command` function that returns the underlying `std::Command`
that would be called to open given path.

Related: Byron#63
Related: helix-editor/helix#5820
matoous added a commit to matoous/open-rs that referenced this pull request Feb 5, 2023
Adds new `command` function that returns the underlying `std::Command`
that would be called to open given path.

Related: Byron#63
Related: helix-editor/helix#5820
matoous added a commit to matoous/open-rs that referenced this pull request Feb 5, 2023
Adds new `command` function that returns the underlying `std::Command`
that would be called to open given path.

Related: Byron#63
Related: helix-editor/helix#5820
matoous added a commit to matoous/open-rs that referenced this pull request Feb 5, 2023
Adds new `command` function that returns the underlying `std::Command`
that would be called to open given path.

Related: Byron#63
Related: helix-editor/helix#5820
@Byron
Copy link
Contributor

Byron commented Mar 6, 2023

A new release of open is now available. Maybe this allows the PR to proceed.

@matoous matoous force-pushed the md/1472-open-url branch 2 times, most recently from 5213c3f to 5a94c36 Compare March 10, 2023 18:16
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 10, 2023
matoous added a commit to matoous/helix that referenced this pull request Nov 19, 2023
Implement [window.showDocument](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument)
LSP server-sent request.

This PR builds on top of helix-editor#5820,
moves the external-URL opening functionality into shared crate-level
function that returns a callback that is now used by both the
`open_file` command as well as the window.showDocument handler if
the URL is marked as external.
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

just some minor comments, otherwise I think this looks good

helix-term/Cargo.toml Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@matoous
Copy link
Contributor Author

matoous commented Nov 21, 2023

@the-mikedavis thank you, addressed.

@David-Else
Copy link
Contributor

David-Else commented Nov 21, 2023

Works great using Firefox on Linux, thanks! For Markdown:

[Tokio](https://tokio.rs/)

Selecting mim (inside the brackets) gf works a treat, this is a massive feature, can't wait to see it merged :)

I noticed these warnings when compiling:

warning: package `cc v1.0.84` in Cargo.lock is yanked in registry `crates-io`, consider running without --locked
warning: package `rustix v0.38.22` in Cargo.lock is yanked in registry `crates-io`, consider running without --locked

I see #8874 rustix was updated very recently.

@pascalkuthe pascalkuthe merged commit 3052050 into helix-editor:master Nov 21, 2023
6 checks passed
matoous added a commit to matoous/helix that referenced this pull request Nov 21, 2023
Implement [window.showDocument](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument)
LSP server-sent request.

This PR builds on top of helix-editor#5820,
moves the external-URL opening functionality into shared crate-level
function that returns a callback that is now used by both the
`open_file` command as well as the window.showDocument handler if
the URL is marked as external.
matoous added a commit to matoous/helix that referenced this pull request Nov 21, 2023
Implement [window.showDocument](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument)
LSP server-sent request.

This PR builds on top of helix-editor#5820,
moves the external-URL opening functionality into shared crate-level
function that returns a callback that is now used by both the
`open_file` command as well as the window.showDocument handler if
the URL is marked as external.
matoous added a commit to matoous/helix that referenced this pull request Dec 13, 2023
Implement [window.showDocument](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument)
LSP server-sent request.

This PR builds on top of helix-editor#5820,
moves the external-URL opening functionality into shared crate-level
function that returns a callback that is now used by both the
`open_file` command as well as the window.showDocument handler if
the URL is marked as external.
matoous added a commit to matoous/helix that referenced this pull request Dec 23, 2023
Implement [window.showDocument](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument)
LSP server-sent request.

This PR builds on top of helix-editor#5820,
moves the external-URL opening functionality into shared crate-level
function that returns a callback that is now used by both the
`open_file` command as well as the window.showDocument handler if
the URL is marked as external.
matoous added a commit to matoous/helix that referenced this pull request Jan 8, 2024
Implement [window.showDocument](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument)
LSP server-sent request.

This PR builds on top of helix-editor#5820,
moves the external-URL opening functionality into shared crate-level
function that returns a callback that is now used by both the
`open_file` command as well as the window.showDocument handler if
the URL is marked as external.
matoous added a commit to matoous/helix that referenced this pull request Jan 8, 2024
Implement [window.showDocument](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument)
LSP server-sent request.

This PR builds on top of helix-editor#5820,
moves the external-URL opening functionality into shared crate-level
function that returns a callback that is now used by both the
`open_file` command as well as the window.showDocument handler if
the URL is marked as external.
pascalkuthe pushed a commit that referenced this pull request Jan 17, 2024
* feat(lsp): implement show document request

Implement [window.showDocument](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument)
LSP server-sent request.

This PR builds on top of #5820,
moves the external-URL opening functionality into shared crate-level
function that returns a callback that is now used by both the
`open_file` command as well as the window.showDocument handler if
the URL is marked as external.

* add return

* use vertical split

* refactor

---------

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
hamrik pushed a commit to hamrik/helix that referenced this pull request Jan 27, 2024
* feat(lsp): implement show document request

Implement [window.showDocument](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument)
LSP server-sent request.

This PR builds on top of helix-editor#5820,
moves the external-URL opening functionality into shared crate-level
function that returns a callback that is now used by both the
`open_file` command as well as the window.showDocument handler if
the URL is marked as external.

* add return

* use vertical split

* refactor

---------

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
dgkf pushed a commit to dgkf/helix that referenced this pull request Jan 30, 2024
* feat(commands): open urls with goto_file command

Add capability for `goto_file` command to open an URL under cursor.

Fixes: helix-editor#1472
Superseds: helix-editor#4398

* open files inside helix

* address code review

* bump deps

* fix based on code review comments
dgkf pushed a commit to dgkf/helix that referenced this pull request Jan 30, 2024
* feat(lsp): implement show document request

Implement [window.showDocument](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument)
LSP server-sent request.

This PR builds on top of helix-editor#5820,
moves the external-URL opening functionality into shared crate-level
function that returns a callback that is now used by both the
`open_file` command as well as the window.showDocument handler if
the URL is marked as external.

* add return

* use vertical split

* refactor

---------

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
* feat(commands): open urls with goto_file command

Add capability for `goto_file` command to open an URL under cursor.

Fixes: helix-editor#1472
Superseds: helix-editor#4398

* open files inside helix

* address code review

* bump deps

* fix based on code review comments
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
* feat(lsp): implement show document request

Implement [window.showDocument](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument)
LSP server-sent request.

This PR builds on top of helix-editor#5820,
moves the external-URL opening functionality into shared crate-level
function that returns a callback that is now used by both the
`open_file` command as well as the window.showDocument handler if
the URL is marked as external.

* add return

* use vertical split

* refactor

---------

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
* feat(commands): open urls with goto_file command

Add capability for `goto_file` command to open an URL under cursor.

Fixes: helix-editor#1472
Superseds: helix-editor#4398

* open files inside helix

* address code review

* bump deps

* fix based on code review comments
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
* feat(lsp): implement show document request

Implement [window.showDocument](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument)
LSP server-sent request.

This PR builds on top of helix-editor#5820,
moves the external-URL opening functionality into shared crate-level
function that returns a callback that is now used by both the
`open_file` command as well as the window.showDocument handler if
the URL is marked as external.

* add return

* use vertical split

* refactor

---------

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
* feat(commands): open urls with goto_file command

Add capability for `goto_file` command to open an URL under cursor.

Fixes: helix-editor#1472
Superseds: helix-editor#4398

* open files inside helix

* address code review

* bump deps

* fix based on code review comments
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
* feat(lsp): implement show document request

Implement [window.showDocument](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument)
LSP server-sent request.

This PR builds on top of helix-editor#5820,
moves the external-URL opening functionality into shared crate-level
function that returns a callback that is now used by both the
`open_file` command as well as the window.showDocument handler if
the URL is marked as external.

* add return

* use vertical split

* refactor

---------

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open URL under cursor
7 participants