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: Update Menu name to use SharedString type to support more types #14791

Merged
merged 3 commits into from
Jul 19, 2024

Conversation

huacnlee
Copy link
Contributor

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jul 19, 2024
@huacnlee
Copy link
Contributor Author

Sorry, I meant to submit a post on my fork first, then create a discussion to see if this suggestion is accepted, but I accidentally created it here.

Let's discuss it here.

@huacnlee
Copy link
Contributor Author

huacnlee commented Jul 19, 2024

I want to change the name type of Menu because I tried to add I18n support to Zed before, and used the rust-i18n crate that I implemented. The t! method in it needs to support runtime strings (such as format!("Close all {} items", 5)), so the return value is of type Cow instead of static's str.

Here is a draft:

huacnlee#3

Or sometime we use GPUI to create app, the Menu name may be a dynamic string that created in runtime, but by current design we can't store it to Menu, so I want to change the type of Menu name to Cow.

@huacnlee huacnlee changed the title gpui: Update Menu's name type to use Cow<'a, str> to support more types. gpui: Update Menu's name type to use Cow<'a, str> to support more types Jul 19, 2024
@@ -6,7 +6,7 @@ use util::ResultExt;
/// A menu of the application, either a main menu or a submenu
pub struct Menu<'a> {
/// The name of the menu
pub name: &'a str,
pub name: Cow<'a, str>,
Copy link
Member

Choose a reason for hiding this comment

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

Have we considered using a SharedString here?

That type is more commonly used in GPUI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SharedString is ok, it can handle same things.

@huacnlee huacnlee changed the title gpui: Update Menu's name type to use Cow<'a, str> to support more types gpui: Update Menu name to use SharedString type to support more types Jul 19, 2024
Copy link
Member

@maxdeviant maxdeviant left a comment

Choose a reason for hiding this comment

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

Thanks, this seems like a useful thing to have!

@maxdeviant maxdeviant merged commit fb541ac into zed-industries:main Jul 19, 2024
10 checks passed
@huacnlee huacnlee deleted the update-menu-action-type branch July 19, 2024 12:52
@huacnlee
Copy link
Contributor Author

huacnlee commented Jul 19, 2024

@maxdeviant And I have almost done the I18n support for Zed.

image

huacnlee#3

Are there any plans to introduce this support at this stage? After the introduction, we need to keep using t!() instead of static str for each display string. But it is simple, I also did backward compatibility. If some translate is missing, the original string will be used for display, which will not have much impact on usage.

This may increase the workload of Zed development.

image

@maxdeviant
Copy link
Member

Are there any plans to introduce this support at this stage?

We haven't really discussed it internally yet, but my guess is that it's probably a ways off.

@huacnlee
Copy link
Contributor Author

Okay, I'll put it aside for now.

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.

3 participants