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

Reveal in files instead of Finder #13432

Merged

Conversation

francesco-gaglione
Copy link
Contributor

@francesco-gaglione francesco-gaglione commented Jun 23, 2024

fixes: #12776

Release Notes:

  • Renamed editor::RevealInFinder to editor::RevealInFileManager

Copy link

cla-bot bot commented Jun 23, 2024

We require contributors to sign our Contributor License Agreement, and we don't have @francesco-gaglione on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

@francesco-gaglione
Copy link
Contributor Author

@cla-bot check

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jun 23, 2024
Copy link

cla-bot bot commented Jun 23, 2024

The cla-bot has been summoned, and re-checked this pull request!

@maxdeviant maxdeviant changed the title reveal in files instead of finder Reveal in files instead of Finder Jun 23, 2024
@mrnugget
Copy link
Member

If we're doing a rename, I think we should use the generic term file manager. Finder/Files are specific application names.

@francesco-gaglione
Copy link
Contributor Author

@mrnugget working on it

@francesco-gaglione
Copy link
Contributor Author

francesco-gaglione commented Jun 24, 2024

Copy link
Contributor

@mikayla-maki mikayla-maki left a comment

Choose a reason for hiding this comment

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

I don't think we should do a rename, I'd prefer if we added actions for each operating system, and then used cfg!()to switch between which actions we register and use in the UI.

@mikayla-maki
Copy link
Contributor

mikayla-maki commented Jun 24, 2024

Essentially, this would mean wrapping each .on_action or .action with something like .when(cfg!(target_os = "OPERATING SYSTEM"), |this| { this.action(/*....*/) })

@francesco-gaglione
Copy link
Contributor Author

@mikayla-maki wrapped actions with your suggestion. Check it if it is enough. Thank you.

@francesco-gaglione
Copy link
Contributor Author

This pr can be merged or I have to solve conflicts first?

@mrnugget
Copy link
Member

mrnugget commented Jul 1, 2024

This pr can be merged or I have to solve conflicts first?

I think rebasing on main (or merging it in) and fixing the conflicts would be great.

@francesco-gaglione
Copy link
Contributor Author

@mrnugget done it. so we can merge?

docs/src/key-bindings.md Outdated Show resolved Hide resolved
docs/src/key-bindings.md Outdated Show resolved Hide resolved
@francesco-gaglione
Copy link
Contributor Author

I don't have a mac can anyone test before merging?

@mrnugget
Copy link
Member

mrnugget commented Jul 9, 2024

I tested on Linux and on macOS. On Linux everything works as expected. On macOS the action is now called "Reveal in File Manager".

I think @mikayla-maki's idea was that we define two actions: RevealInFinder and RevealInFileManager and only use one of two, depending on the target dir. (Also: reveal_in_finder is still the method name.)

The problem is that for the vim.json keybindings we have to pick one over the other and we can't do a runtime check of vim.enabled to define one or the other action.

Sooooo, should we go with the rename then, @mikayla-maki? :D

Copy link
Contributor

@mikayla-maki mikayla-maki left a comment

Choose a reason for hiding this comment

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

@mrnugget Agreed! Thanks for bringing that issue to my attention :)

@mikayla-maki mikayla-maki merged commit 2dd4867 into zed-industries:main Jul 9, 2024
10 checks passed
JosephTLyons pushed a commit that referenced this pull request Jul 9, 2024
fixes: #12776

Release Notes:

- Renamed `editor::RevealInFinder` to `editor::RevealInFileManager`

---------

Co-authored-by: Mikayla Maki <[email protected]>
@francesco-gaglione francesco-gaglione deleted the reveal-in-files branch July 10, 2024 20:37
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.

Reveal in finder refer to mac os and on linux or other
5 participants