Icon rework

[andreaskrath]

I have been looking over the approach to icons for a bit and I think there are some improvements to be made, this issue contains my suggestions.

The current implementation is pretty "locked" in its implementation, as it maps 82 enum variants to embedded asset paths. This leaves two main issues:

1. If you want more than 82 icons in your application, you have a problem
2. If you want an icon in your application which does not have an appropriate IconName, you must pick a "spare" option and remember this mapping throughout your application

Another, but somewhat separate issue is that multiple components utilize icons internally, which are only rendered when the icon is present in the loaded assets, even though the button that "contains" the icon is still rendered. My best example of this is the close button for a sheet, which is just a Ghost variant button with no text or icon, making it difficult, if not very unlikely that you actually discover (unless you subscribe to log events and see the errors for failing to get the icon). I personally think it would make sense to always have these icons available in the crate, as they are "essential" in some way.

My suggestion is to add a new public trait that requires a .path method to be implemented. The Icon is changed to take anything that implements this trait as input. Then this trait can be utilized by users of this crate to implement their "own" IconName, which can contain however many icons they like, with whatever name they desire. The current IconName from this crate can still be kept as a sensible default.

This way, the essential icons could also be embedded and enabled elsewhere, outside of the current IconName, to ensure they would not interfere with users of this crate (although that may go against the desired behaviour of overwriting these essential icons).

I think an implementation of just the trait would be quite simple, and if it sounds reasonable and is desired I can implement it.

[huacnlee]

Would please make PR for a simple example to explain your mind?

I know the limitation of the IconName design, but I have no idea how to improve this.

In our application we have another one named IconNamed for addition icons of the application needs

[zanmato]

You can already use your own IconName, like this:

#[derive(Clone)]
pub enum MyIconName {
    Wrench,
}

impl MyIconName {
    pub fn path(self) -> SharedString {
        match self {
            Self::Wrench => "icons/wrench.svg",
        }
        .into()
    }

    pub fn view(self, cx: &mut App) -> Entity<gpui_component::Icon> {
        gpui_component::Icon::empty().path(self.path()).view(cx)
    }
}

impl From<MyIconName> for SharedString {
    fn from(val: MyIconName) -> Self {
        val.path()
    }
}

impl From<MyIconName> for gpui_component::Icon {
    fn from(val: MyIconName) -> Self {
        gpui_component::Icon::empty().path(val.path())
    }
}

Then you use MyIconName::Wrench wherever it expects IconName. You also need to embed your own assets, like in the docs: https://longbridge.github.io/gpui-component/docs/assets#build-you-own-assets

[andreaskrath]

@zanmato I personally think this approach is a rather roundabout way of implementing your own IconName.

The following would be a relatively simple change which would broaden the usage of the IconNamed trait (which I do not think has any usage in the codebase at the moment).

pub trait IconNamed {
    fn path(self) -> SharedString;
}

impl<T: IconNamed> From<T> for Icon {
    fn from(value: T) -> Self {
        Icon::build(value)
    }
}

impl Icon {
    fn build(name: impl IconNamed) -> Self {
        Self::default().path(name.path())
    }

This would require that the existing IconName in the crate has its path method implemented as part of the IconNamed trait, but this is trivial.

The only issue I encounter is specifically when the IconName variant is utilized directly via as a child, like .child(IconName::ChevronRight) (from here. I am not sure if this is a common pattern, but it happens throughout the crate a few times. For this to be possible, the IconName itself must implement IntoElement, and this is not possible to do broadly because T is not a local type (orphan rule).

// RenderOnce is required to be implemented to derive IntoElement on IconName
impl<T: IconNamed> RenderOnce for T {
    fn render(self, _: &mut Window, _: &mut App) -> impl IntoElement {
        Icon::build(self)
    }
}

However, this implementation can still be left for the internal IconName if this behavior is desirable. It would just have to be an additional trait implementation for a user of the crate to utilize this specific pattern.

Technically this also counts for the existing implementation of IconName for AnyElement - however, to my knowledge this does not have any use internally in the crate; so the effect of this is unclear for me.

[zanmato]

@andreaskrath Agreed! I just meant that it is possible, you don't have to pick a "spare icon" like you wrote in the issue.

Regarding .child(IconName) I think that it could be patched if you open a PR with your changes, just a few lines above it is using Icon::new(), https://github.com/longbridge/gpui-component/blob/9d11418bd15c6fd465ef3fd8b674f1544e13039d/crates/ui/src/menu/popup_menu.rs#L1122-L1124 so that approach should be acceptable I guess.

[andreaskrath]

From what I gather the Icon::new(IconName::Variant) approach is utilized when styling is applied to the icon, while in the other cases, where the icon is "raw" the IconName::Variant approach is utilized.