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.

Sign up for GitHub

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

Jump to bottom

Rename the OpenFile action to OpenSelectedFilename to better reflect its function #22494

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Rename the OpenFile action to OpenSelectedFilename to better reflect its function #22494

wants to merge 1 commit into from

Conversation

cole-miller
Copy link
Contributor

@cole-miller cole-miller commented Dec 30, 2024 •
edited
Loading

Release Notes:

  • Renamed the OpenFile action to OpenSelectedFilename for clarity

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Dec 30, 2024
@cole-miller
Copy link
Contributor Author

This preserves the ability to refer to editor::OpenFile in config files for backward compatibility, but only surfaces the new name in UI (suggestions when editing config, and the command palette).

@mrnugget does this make sense to you? In #22250 a user was expecting the action to open a file picker, this is intended to prevent that kind of confusion.

@mgsloan
Copy link
Contributor

mgsloan commented Dec 30, 2024 •
edited
Loading

Rename makes sense to me! Added a comment about implementation particulars

mgsloan
mgsloan reviewed Dec 30, 2024
View reviewed changes
crates/editor/src/actions.rs Outdated
@@ -391,3 +390,5 @@ gpui::actions!(
action_as!(outline, ToggleOutline as Toggle);

action_as!(go_to_line, ToggleGoToLine as Toggle);

action_as!(editor, OpenSelectedFilename as [OpenFile]);
Copy link
Contributor

@mgsloan mgsloan Dec 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's confusing to use as for these two very different meanings. Not sure what to do instead though. Maybe just a new macro that takes a list of string aliases?

An alternative might also be to clarify that the purpose of these aliases is to not break user's configuration files. We may want to even deprecate them / auto-migrate configs off them to encourage homogeneity.

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 like the auto-migration idea!

Copy link
Contributor Author

@cole-miller cole-miller Dec 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Pushed a fresh attempt, using deprecation metadata in the JSON Schema we provide for keymap files so that the language server will surface this information in a helpful way. Also renamed the macro. I did not try to implement auto-migration.

@cole-miller cole-miller force-pushed the cole/rename-open-file branch from 30299ed to 202aefd Compare December 31, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgsloan mgsloan mgsloan left review comments

Assignees
No one assigned
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cole-miller @mgsloan