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

Fix issues with opening links and files using WSL #3850

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bottino
Copy link

@bottino bottino commented Aug 23, 2024

  • PR Description

This PR fixes to issues with the current implementation of the OSConfig for WSL:

File explorer
When using the file explorer from WSL, for instance by pressing "o" in the "Files" menu, the explorer always opens on the default landing page. This is caused by the filepath being expressed in WSL format instead of the expected Windows format. For instance:

  • "/home/myuser/path/to/my/file" should be "\wsl$\MyDistroName\home\myuser\path\to\my\file"
  • "/mnt/c/path/to/my/file" should be "C:\path\to\my\file"

There's a utility to do that in WSL, wslpath. We use it in the Open to format the filename before passing it to the Powershell command

Link URLs
Opening links containing ampersands inside lazygit (a pull-request creation page in BitBucket Server, for instance) returns the following Powershell error:

The ampersand (&) character is not allowed. The & operator is reserved
for future use; wrap an ampersand in double quotation marks ("&") to
pass it as part of a string.

We fix it by enclosing the URL in single quotes.

Notes on this PR
This is my first PR on this repo, please tell me if something is needed. I read the contributing guide.
The OS-specific logic doesn't appear to be tested in integration and unit tests, so I didn't add tests.

  • Please check if the PR fulfills these requirements
  • Cheatsheets are up-to-date (run go generate ./...)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
  • Docs have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

@bottino
Copy link
Author

bottino commented Sep 1, 2024

As an alternative, we could default to using xdg-open for all Linux systems, and refer WSL users to wsl-open.

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

I'm not well versed in WSL but I trust your reasoning.

@jesseduffield jesseduffield added the bug Something isn't working label Nov 9, 2024
The OS command to open file in explorer in WSL doesn't currently work as
expected; it always opens the file explorer at the default opening
location. This is because the {{filename}} variable returns the path in
WSL format, and not in the format expected by Windows.

We use wslpath, a utility shipped with WSL, to make the path conversion.
Opening links containing ampersands inside lazygit (a pull-request
creation page in BitBucket Server, for instance) returns the following
Powershell error:
> The ampersand (&) character is not allowed. The & operator is reserved
> for future use; wrap an ampersand in double quotation marks ("&") to
> pass it as part of a string.

We fix it by enclosing the URL in single quotes.
@bottino
Copy link
Author

bottino commented Nov 14, 2024

@jesseduffield Is something wrong with the checks? They don't seem to run.

@bottino
Copy link
Author

bottino commented Nov 14, 2024

This PR will fix #3843

@mark2185
Copy link
Collaborator

@jesseduffield Is something wrong with the checks? They don't seem to run.

First time contributors have to have their changes approved manually before the CI runs them.

@bottino
Copy link
Author

bottino commented Nov 19, 2024

Do I still need to do something to merge this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants