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

gpui: Add platform-specific keystroke display #25651

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

KorigamiK
Copy link
Contributor

@KorigamiK KorigamiK commented Feb 26, 2025

Closes #21938

I think I accidentally closed #23979 while pulling the latest so I had to create a new branch for it.

Release Notes:

  • Added: platform specific keystroke display

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Feb 26, 2025
@zed-industries-bot
Copy link

Warnings
⚠️

This PR is missing release notes.

Please add a "Release Notes" section that describes the change:

Release Notes:

- Added/Fixed/Improved ...

If your change is not user-facing, you can use "N/A" for the entry:

Release Notes:

- N/A

Generated by 🚫 dangerJS against c95bcc2

@maxdeviant maxdeviant changed the title gpui: Add platform-specific keystroke display gpui: Add platform-specific keystroke display Feb 26, 2025
Comment on lines 354 to 366
fn open_with_system(&self, path: &Path) {
let executor = self.background_executor().clone();
let path = path.to_owned();
executor
.spawn(async move {
let _ = std::process::Command::new("xdg-open")
.arg(path)
.spawn()
.expect("Failed to open file with xdg-open");
})
.detach();
}

Copy link

Choose a reason for hiding this comment

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

Don't hard-code shelling out to a program that isn't even listed as a dependency of the crate. The right way to do this is to use a library that understands mime associations (e.g. https://docs.rs/xdg-utils/latest/xdg_utils/). In the worst case the command should be configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree, i used this because it's standard command similar to open on macos (although might have to be explicitly installed on linux).
perhaps this is only a linux specific dependency which i can add.

should i create a separate pr for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alt key icon is incorrect on Linux
3 participants