Repository navigation
icon: Add independently embedded SVG constants - #3360
suxiaoshao wants to merge 2 commits into
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
huacnlee
left a comment
There was a problem hiding this comment.
I understand the motivation: keeping the icon_assets! selection list in sync with Rust use sites requires separate maintenance, and stale entries can retain unused SVG payloads. SvgIcon makes resource inclusion follow reachable Rust references instead.
However, I do not think this benefit justifies introducing another public icon API. gpui-kit-assets and IconName already provide the shared catalog and its selection/loading mechanisms. Exposing the same catalog through SvgIcon and another set of constants duplicates that functionality and adds long-term API maintenance cost.
Please reconsider this approach rather than proceeding with the additional SvgIcon API. If selection-list maintenance is a significant practical problem, explore a solution within the existing assets and IconName design, or provide a concrete use case demonstrating why that design cannot reasonably address it. The objection is to the proposed product/API scope, not an implementation correctness defect.
Please address the findings above, then request my review again.
Closes #3349
Description
Using extra catalog icons through
icon_assets!requires keeping a selection list in sync with the Rust use sites. Removing the last UI reference can leave an obsolete entry, and its SVG bytes, in the runtime asset source.Add
SvgIconas a third option alongside the existing full-catalog and macro-selected asset sources. Each generated constant references only its own embedded SVG, so optimized builds can drop unreferenced payloads without an application-maintained list:The asset crate generates the catalog from its existing SVG files; Component owns the new value type and presentation conversion. Direct children, entity views, custom static SVGs, component icon slots, and native-menu byte resolution are supported. No new dependency or application asset-source registration is required for these icons.
Existing
IconNameenums, customIconNamedimplementations, defaultAssets,AllAssets, andicon_assets!retain their behavior. Components' internal path-based icons still need the normal default assets. The new byte-backed constants embed their selected SVGs on native and WASM targets; CDN loading remains available through existing path sources. English/Chinese documentation and the Icon Story demonstrate the additional option.Current copying cost and upstream follow-up
This implementation intentionally uses today's APIs: conversion calls
Icon::data(&[u8]), copying into anArc<[u8]>; building the underlying GPUISvgcallsSvg::data(&[u8]), copying again and preparing its content cache key. These are element-construction costs; cached rendering does not imply parsing/rasterization on every paint. No application-level performance improvement is claimed.Related upstream work: zed#65066 and zed#65067. If reusable
SvgDatabecomes available in the GPUI dependency, a localized follow-up can store it in privateIconSource::Data, construct static data inFrom<SvgIcon>, forward it directly intoSvg, and useas_bytes()for native menus.Icon::new(SvgIcon::...)remains unchanged, and the existingIcon::data(&[u8])signature can retain its copying semantics. No breaking public API migration is required for that optimization.Public API
gpui-component
All items are exposed through
gpui_kit::componentas well.pub struct SvgIcon { /* private fields */ }— copyable embedded SVG value; derivesClone,Copy,Debug,Eq,PartialEq,Hash, andIntoElement.pub const fn SvgIcon::new(bytes: &'static [u8]) -> Self— wraps custom static SVG bytes.pub const fn SvgIcon::bytes(self) -> &'static [u8]— returns the original bytes.pub fn SvgIcon::view(self, cx: &mut App) -> Entity<Icon>— constructs an entity-backed icon.impl From<SvgIcon> for Icon— accepts embedded icons in existing component icon slots.impl RenderOnce for SvgIcon— supports direct element-tree composition through the existing Icon presentation.impl From<SvgIcon> for AnyElement— supports type-erased element conversion.pub const SvgIcon::<NAME>: Self— each independently references the SVG identified by its uppercase, underscore-separated file stem. The complete identifier list is below; there is no runtime catalog array or lookup table in this API.All 1830 generated SvgIcon constants (each: pub const NAME: Self)
gpui-kit-assets
__component_svg_icons!($icon:ty)— new#[doc(hidden)]exported macro generating Component's associated constants across the crate boundary. This is explicitly internal, not a supported application extension point.No existing public item is changed or removed; no JavaScript or TypeScript API changes.
How to Test
Validated on macOS:
cargo check -p gpui-component -p gpui-kit --lockedcargo test -p gpui-component --lib icon --locked— 20 passed, including byte conversion and native-menu resolution.cargo test -p gpui-kit --test assets --locked— 3 passed, including old enums, customIconNamed, and new icon slots.cargo test -p gpui-kit-assets --test icons --locked— 5 passed for the existing catalog, defaults, and selection macro.cargo clippy -p gpui-component -p gpui-kit --lib --locked -- --deny warningscargo check -p gpui-component-story --lockedcargo run -p gpui-component-story --locked -- Icon— built and launched. Window automation could not identify the standalone process, so visual appearance has not been confirmed. Manually inspect the new “Embedded catalog icons” section and its “Set alarm” button.rustfmt --check; changed API/docs passedtypos;git diff --checkpassed.A temporary external package depending on this Kit checkout was built with
cargo build --release --offline, with symbol stripping and no explicit LTO override. Itsmainpassed eachIcon::new(SvgIcon::...)throughstd::hint::black_box. Scanning each executable for every catalog SVG's complete bytes produced:ACCESSIBILITYACCESSIBILITY,ALARM_CLOCKALARM_CLOCKThe byte scan, rather than total size alone, confirms removal: alignment can hide payload-size differences. These figures describe the small probe, not a full application's size or runtime memory. The temporary verification script is not included in this PR. Windows/Linux/WASM execution was not performed locally.
Checklist
Implementation, tests, and documentation were written with Codex assistance.