svg_img: Refactor SvgImg to avoid assign source image size and fix drop image error. (#821)

## Break Change

- Removed `source` method from `SvgImg`, now need assign source in `new`
method.
- There is no need to give image size, it will read the size from SVG
file.

crates/story/src/fixtures/color-wheel.svg

@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="iso-8859-1" ?>
+<!-- Uploaded to: SVG Repo, www.svgrepo.com, Generator: SVG Repo Mixer Tools -->
+<svg
+    version="1.1"
+    id="Layer_1"
+    xmlns="http://www.w3.org/2000/svg"
+    xmlns:xlink="http://www.w3.org/1999/xlink"
+    viewBox="0 0 512 512"
+    xml:space="preserve"
+>
+<path
+        style="fill:#2BA5F7;"
+        d="M426.578,330.454c-0.574,93.717-76.724,169.515-170.571,169.515
+	c-93.86,0-170.01-75.798-170.584-169.515c25.066,14.463,54.149-1.069,85.162-1.069c31.13,0,60.304-30.644,85.422-45.211
+	c25.118,14.567,54.292,45.211,85.422,45.211C372.442,329.385,401.525,344.918,426.578,330.454z"
+    />
+<path
+        style="fill:#F95428;"
+        d="M512,182.615c0,63.2-34.365,118.366-85.422,147.84c0-0.352,0-0.717,0-1.069
+	c0-63.187-67.386-85.318-118.444-114.805c-0.391-62.639-1.5-150.304-52.127-179.648c25.118-14.567,54.292-22.901,85.422-22.901
+	C435.628,12.031,512,88.402,512,182.615z"
+    />
+<path
+        style="fill:#F7B239;"
+        d="M256.007,34.932c-50.627,29.344,9.086,139.653,8.695,202.292
+	c-51.058,29.474-179.279,28.961-179.279,92.161c0,0.352,0,0.717,0,1.069C34.365,300.98,0,245.815,0,182.615
+	C0,88.402,76.372,12.031,170.584,12.031C201.714,12.031,230.888,20.364,256.007,34.932z"
+    />
+<path
+        style="fill:#666666;"
+        d="M256.007,143.325c31.013,0,60.096,23.758,85.162,38.221c0,0.352,0,0.717,0,1.069
+	c0,31.541-3.289,77.082-18.215,102.429c-14.926,25.346-41.489,30.498-66.946,45.254c-26.65-15.448-64.993-33.093-79.988-60.034
+	c-13.653-24.529-5.174-57.586-5.174-87.649c0-0.352,0-0.717,0-1.069C195.911,167.082,224.994,143.325,256.007,143.325z"
+    />
+<path
+        style="fill:#2197D8;"
+        d="M426.578,329.385c0,0.352,0,0.717,0,1.069c-25.053,14.463-54.136,22.745-85.149,22.745
+	c-31.13,0-60.304-8.334-85.422-22.901c50.914-29.513,85.162-84.601,85.162-147.683c0-0.352,0-0.717,0-1.069
+	C392.226,211.033,426.578,266.199,426.578,329.385z"
+    />
+<path
+        style="fill:#E09B2D;"
+        d="M170.845,182.615c0,63.082,34.247,118.17,85.162,147.683c-25.118,14.567-54.292,22.901-85.422,22.901
+	c-31.013,0-60.096-8.281-85.162-22.745c0-0.352,0-0.717,0-1.069c0-63.2,34.365-118.366,85.422-147.84
+	C170.845,181.898,170.845,182.263,170.845,182.615z"
+    />
+<path
+        style="fill:#E54728;"
+        d="M341.168,181.546c-25.066-14.463-54.149-22.745-85.162-22.745s-60.096,8.281-85.162,22.745
+	c0.391-62.639,34.534-117.27,85.162-146.614C306.634,64.276,340.777,118.907,341.168,181.546z"
+    />
+</svg>

crates/story/src/image_story.rs

@@ -1,14 +1,18 @@
 use gpui::{
-    px, App, AppContext, ElementId, Entity, FocusHandle, Focusable, ParentElement as _, Render,
+    img, App, AppContext, ClickEvent, ElementId, Entity, FocusHandle, Focusable,
-    Styled, Window,
+    ParentElement as _, Render, Styled, Window,
 };
-use gpui_component::{dock::PanelControl, v_flex, SvgImg};
+use gpui_component::{button::Button, dock::PanelControl, v_flex, SvgImg};
 
 use crate::section;
 
-const GOOGLE_LOGO: &str = include_str!("./fixtures/google.svg");
+const SVG_ITEMS: &[&str] = &[
+    include_str!("./fixtures/google.svg"),
+    include_str!("./fixtures/color-wheel.svg"),
+];
 
 pub struct ImageStory {
+    svg_index: usize,
     focus_handle: gpui::FocusHandle,
 }
 
@@ -33,6 +37,7 @@ impl super::Story for ImageStory {
 impl ImageStory {
     pub fn new(_: &mut Window, cx: &mut App) -> Self {
         Self {
+            svg_index: 0,
             focus_handle: cx.focus_handle(),
         }
     }
@@ -40,6 +45,10 @@ impl ImageStory {
     pub fn view(window: &mut Window, cx: &mut App) -> Entity<Self> {
         cx.new(|cx| Self::new(window, cx))
     }
+
+    fn svg_img(&self, id: impl Into<ElementId>) -> SvgImg {
+        SvgImg::new(id, SVG_ITEMS[self.svg_index].as_bytes())
+    }
 }
 
 impl Focusable for ImageStory {
@@ -52,19 +61,29 @@ impl Render for ImageStory {
     fn render(
         &mut self,
         _window: &mut gpui::Window,
-        _: &mut gpui::Context<Self>,
+        cx: &mut gpui::Context<Self>,
     ) -> impl gpui::IntoElement {
-        v_flex().gap_4().size_full().child(
+        v_flex()
-            section("SVG Image")
+            .gap_4()
-                .child(svg_img("logo1").size(px(100.)).flex_grow())
+            .size_full()
-                .child(svg_img("logo2").size(px(100.)).flex_grow())
+            .child(
-                .child(svg_img("logo3").size_80().flex_grow())
+                Button::new("switch")
-                .child(svg_img("logo4").size_12().flex_grow())
+                    .label("Switch SVG")
-                .child(svg_img("logo5").size(px(100.))),
+                    .on_click(cx.listener(|this, _: &ClickEvent, _, cx| {
-        )
+                        this.svg_index += 1;
+                        if this.svg_index >= SVG_ITEMS.len() {
+                            this.svg_index = 0;
+                        }
+                        cx.notify();
+                    })),
+            )
+            .child(section("SVG 160px").child(self.svg_img("logo1").size_40().flex_grow()))
+            .child(section("SVG 80px").child(self.svg_img("logo3").size_20().flex_grow()))
+            .child(section("SVG 48px").child(self.svg_img("logo4").size_12().flex_grow()))
+            .child(
+                section("SVG from img 40px").child(
+                    img("https://pub.lbkrs.com/files/202503/vEnnmgUM6bo362ya/sdk.svg").h_24(),
+                ),
+            )
     }
 }
-
-fn svg_img(id: impl Into<ElementId>) -> SvgImg {
-    SvgImg::new(id).source(GOOGLE_LOGO.as_bytes(), px(300.), px(300.))
-}

crates/ui/src/svg_img.rs

@@ -1,13 +1,14 @@
 use std::{
+    collections::HashSet,
     hash::Hash,
     ops::Deref,
-    sync::{Arc, LazyLock},
+    sync::{Arc, LazyLock, Mutex},
 };
 
 use gpui::{
-    hash, px, size, App, Asset, Bounds, Element, ElementId, GlobalElementId, Hitbox,
+    hash, px, App, Asset, Bounds, Element, ElementId, GlobalElementId, Hitbox, ImageCacheError,
-    ImageCacheError, InteractiveElement, Interactivity, IntoElement, IsZero, Pixels, RenderImage,
+    InteractiveElement, Interactivity, IntoElement, Pixels, RenderImage, SharedString, Size,
-    SharedString, Size, StyleRefinement, Styled, Window,
+    StyleRefinement, Styled, Window,
 };
 use image::Frame;
 use smallvec::SmallVec;
@@ -16,8 +17,6 @@ use image::ImageBuffer;
 
 const SCALE: f32 = 2.;
 
-struct SvgImgState(Option<(u64, Arc<RenderImage>)>);
-
 static OPTIONS: LazyLock<usvg::Options> = LazyLock::new(|| {
     let mut options = usvg::Options::default();
     options.fontdb_mut().load_system_fonts();
@@ -62,17 +61,15 @@ impl Clone for SvgImg {
             id: self.id.clone(),
             interactivity: Interactivity::default(),
             source: self.source.clone(),
-            size: self.size,
         }
     }
 }
 
-pub enum Image {}
+enum SvgImageLoader {}
 
 #[derive(Debug, Clone)]
 pub struct ImageSource {
     source: SvgSource,
-    size: Size<Pixels>,
 }
 
 impl Hash for ImageSource {
@@ -82,9 +79,43 @@ impl Hash for ImageSource {
     }
 }
 
-impl Asset for Image {
+#[derive(Debug, Clone)]
+pub struct ImageData {
+    image: Arc<RenderImage>,
+    size: Size<Pixels>,
+    source: ImageSource,
+    uses: Arc<Mutex<HashSet<u64>>>,
+}
+
+impl ImageData {
+    fn new(source: ImageSource, image: Arc<RenderImage>, size: Size<Pixels>) -> Self {
+        Self {
+            source,
+            image,
+            size,
+            uses: Arc::new(Mutex::new(HashSet::new())),
+        }
+    }
+
+    fn use_image(&self, global_id: &GlobalElementId) -> Arc<RenderImage> {
+        self.uses.lock().unwrap().insert(hash(global_id));
+        self.image.clone()
+    }
+
+    fn remove_use(&mut self, global_id: &GlobalElementId, window: &mut Window, cx: &mut App) {
+        let mut uses = self.uses.lock().unwrap();
+        uses.remove(&hash(global_id));
+        if uses.len() == 0 {
+            // println!("dropping image: {:?}", self.image);
+            cx.remove_asset::<SvgImageLoader>(&self.source);
+            _ = window.drop_image(self.image.clone());
+        }
+    }
+}
+
+impl Asset for SvgImageLoader {
     type Source = ImageSource;
-    type Output = Result<Arc<RenderImage>, ImageCacheError>;
+    type Output = Result<ImageData, ImageCacheError>;
 
     fn load(
         source: Self::Source,
@@ -93,16 +124,7 @@ impl Asset for Image {
         let asset_source = cx.asset_source().clone();
 
         async move {
-            let size = source.size;
+            let bytes = match source.source.clone() {
-            if size.width.is_zero() || size.height.is_zero() {
-                return Err(usvg::Error::InvalidSize.into());
-            }
-            let size = Size {
-                width: (size.width * SCALE).ceil(),
-                height: (size.height * SCALE).ceil(),
-            };
-
-            let bytes = match source.source {
                 SvgSource::Data(data) => data,
                 SvgSource::Path(path) => {
                     if let Ok(Some(data)) = asset_source.load(&path) {
@@ -119,10 +141,15 @@ impl Asset for Image {
 
             let tree = usvg::Tree::from_data(&bytes, &OPTIONS)?;
 
-            let mut pixmap =
+            // Get svg size
-                resvg::tiny_skia::Pixmap::new(size.width.0 as u32, size.height.0 as u32)
+            let svg_size = tree.size();
-                    .ok_or(usvg::Error::InvalidSize)?;
+            let mut pixmap = resvg::tiny_skia::Pixmap::new(
+                (svg_size.width() * SCALE) as u32,
+                (svg_size.height() * SCALE) as u32,
+            )
+            .ok_or(usvg::Error::InvalidSize)?;
 
+            let img_size = gpui::size(px(pixmap.width() as f32), px(pixmap.height() as f32));
             let transform = resvg::tiny_skia::Transform::from_scale(SCALE, SCALE);
 
             resvg::render(&tree, transform, &mut pixmap.as_mut());
@@ -141,10 +168,8 @@ impl Asset for Image {
                 }
             }
 
-            Ok(Arc::new(RenderImage::new(SmallVec::from_elem(
+            let image = Arc::new(RenderImage::new(SmallVec::from_elem(Frame::new(buffer), 1)));
-                Frame::new(buffer),
+            Ok(ImageData::new(source, image, img_size))
-                1,
-            ))))
         }
     }
 }
@@ -152,44 +177,26 @@ impl Asset for Image {
 pub struct SvgImg {
     id: ElementId,
     interactivity: Interactivity,
-    source: Option<ImageSource>,
+    source: ImageSource,
-    size: Size<Pixels>,
 }
 
 impl SvgImg {
     /// Create a new svg image element.
     ///
-    /// The `src_width` and `src_height` are the original width and height of the svg image.
+    /// The `source` can be a string of SVG XML data or a Asset Path.
-    pub fn new(id: impl Into<ElementId>) -> Self {
+    pub fn new(id: impl Into<ElementId>, source: impl Into<SvgSource>) -> Self {
         Self {
             id: id.into(),
             interactivity: Interactivity::default(),
-            source: None,
+            source: ImageSource {
-            size: Size::default(),
+                source: source.into(),
+            },
         }
     }
 
-    /// Set the path of the svg image from the asset.
+    /// Get the source of the svg image.
-    ///
+    pub fn source(&self) -> &ImageSource {
-    /// The `size` argument is the size of the original svg image.
+        &self.source
-    #[must_use]
-    pub fn source(
-        mut self,
-        source: impl Into<SvgSource>,
-        width: impl Into<Pixels>,
-        height: impl Into<Pixels>,
-    ) -> Self {
-        let size = size(width.into(), height.into());
-        self.size = size;
-        self.source = Some(ImageSource {
-            source: source.into(),
-            size,
-        });
-        self
-    }
-
-    pub fn get_source(&self) -> Option<&ImageSource> {
-        self.source.as_ref()
     }
 }
 
@@ -201,9 +208,11 @@ impl IntoElement for SvgImg {
     }
 }
 
+struct SvgImgState(Option<(u64, ImageData)>);
+
 impl Element for SvgImg {
-    type RequestLayoutState = Option<Arc<RenderImage>>;
+    type RequestLayoutState = Option<ImageData>;
-    type PrepaintState = (Option<Hitbox>, Option<Arc<RenderImage>>);
+    type PrepaintState = (Option<Hitbox>, Option<ImageData>);
 
     fn id(&self) -> Option<ElementId> {
         Some(self.id.clone())
@@ -215,40 +224,45 @@ impl Element for SvgImg {
         window: &mut Window,
         cx: &mut App,
     ) -> (gpui::LayoutId, Self::RequestLayoutState) {
-        let layout_id =
-            self.interactivity
-                .request_layout(global_id, window, cx, |style, window, cx| {
-                    window.request_layout(style, None, cx)
-                });
         let global_id = global_id.unwrap();
 
+        let layout_id =
+            self.interactivity
+                .request_layout(Some(global_id), window, cx, |style, window, cx| {
+                    window.request_layout(style, None, cx)
+                });
+
+        let source = &self.source;
+        let source_hash = hash(source);
+
         window.with_element_state::<SvgImgState, _>(global_id, |state, window| {
-            match (state, &self.source) {
+            match state {
-                (_, None) => ((layout_id, None), SvgImgState(None)),
+                Some(mut state) => {
-                (Some(SvgImgState(Some((prev_hash, image)))), Some(source))
+                    if let Some((prev_hash, mut prev_image)) = state.0.take() {
-                    if hash(source) == prev_hash =>
+                        if source_hash == prev_hash {
-                {
+                            return (
-                    (
+                                (layout_id, Some(prev_image.clone())),
-                        (layout_id, Some(image.clone())),
+                                SvgImgState(Some((prev_hash, prev_image))),
-                        SvgImgState(Some((prev_hash, image))),
+                            );
-                    )
+                        } else {
-                }
+                            // Drop the previous image from the cache.
-                (state, Some(source)) => {
+                            // Here can't remove directly, because same the image is being used by another element.
-                    if let Some(SvgImgState(Some((_, prev_image)))) = state {
+                            prev_image.remove_use(global_id, window, cx);
-                        // Drop the previous image from the cache
+                        }
-                        _ = window.drop_image(prev_image);
                     }
 
                     let image = window
-                        .use_asset::<Image>(&source, cx)
+                        .use_asset::<SvgImageLoader>(&source, cx)
                         .transpose()
                         .ok()
                         .flatten();
+
                     (
                         (layout_id, image.clone()),
-                        SvgImgState(image.map(|image| (hash(source), image))),
+                        SvgImgState(image.map(|image| (source_hash, image))),
                     )
                 }
+                None => ((layout_id, None), SvgImgState(None)),
             }
         })
     }
@@ -282,43 +296,45 @@ impl Element for SvgImg {
         window: &mut Window,
         cx: &mut App,
     ) {
-        let size = self.size;
         let hitbox = state.0.as_ref();
-        let data = state.1.clone();
+        let Some(image_data) = state.1.take() else {
+            return;
+        };
+        let size = image_data.size;
 
         self.interactivity
             .paint(global_id, bounds, hitbox, window, cx, |_, window, _| {
-                if let Some(data) = data {
+                // To calculate the ratio of the original image size to the container bounds size.
-                    // To calculate the ratio of the original image size to the container bounds size.
+                // Scale by shortest side (width or height) to get a fit image.
-                    // Scale by shortest side (width or height) to get a fit image.
+                // And center the image in the container bounds.
-                    // And center the image in the container bounds.
+                let ratio = if bounds.size.width < bounds.size.height {
-                    let ratio = if bounds.size.width < bounds.size.height {
+                    bounds.size.width / size.width
-                        bounds.size.width / size.width
+                } else {
-                    } else {
+                    bounds.size.height / size.height
-                        bounds.size.height / size.height
+                };
-                    };
 
-                    let ratio = ratio.min(1.0);
+                let ratio = ratio.min(1.0);
+                let new_size = size.map(|dim| dim * ratio);
 
-                    let new_size = gpui::Size {
+                let new_origin = gpui::Point {
-                        width: size.width * ratio,
+                    x: bounds.origin.x + px(((bounds.size.width - new_size.width) / 2.).into()),
-                        height: size.height * ratio,
+                    y: bounds.origin.y + px(((bounds.size.height - new_size.height) / 2.).into()),
-                    };
+                };
-                    let new_origin = gpui::Point {
-                        x: bounds.origin.x + px(((bounds.size.width - new_size.width) / 2.).into()),
-                        y: bounds.origin.y
-                            + px(((bounds.size.height - new_size.height) / 2.).into()),
-                    };
 
-                    let img_bounds = Bounds {
+                let img_bounds = Bounds {
-                        origin: new_origin.map(|origin| origin.floor()),
+                    origin: new_origin.map(|origin| origin.floor()),
-                        size: new_size.map(|size| size.ceil()),
+                    size: new_size.map(|size| size.ceil()),
-                    };
+                };
 
-                    match window.paint_image(img_bounds, px(0.).into(), data, 0, false) {
+                match window.paint_image(
-                        Ok(_) => {}
+                    img_bounds,
-                        Err(err) => eprintln!("failed to paint svg image: {:?}", err),
+                    px(0.).into(),
-                    }
+                    image_data.use_image(&global_id.unwrap()),
+                    0,
+                    false,
+                ) {
+                    Ok(_) => {}
+                    Err(err) => eprintln!("failed to paint svg image: {:?}", err),
                 }
             })
     }