From e4262f97af6f0cfe4af7459dc0f617ae6ae4029d Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Tue, 20 May 2025 15:38:24 +0300 Subject: [PATCH] Restore the ability to drag and drop images into the editor (#31009) `ImageItem`'s `file` is returning `""` as its `path` for single-filed worktrees like the ones are created for the images dropped from the OS. `ImageItem::load_image_metadata` had used that `path` in FS operations and the other method tried to use for icon resolving. Rework the code to use a more specific, `worktree::File` instead and always use the `abs_path` when dealing with paths from this `file`. Release Notes: - Fixed images not opening on drag and drop into the editor --- Cargo.lock | 1 + crates/image_viewer/Cargo.toml | 1 + crates/image_viewer/src/image_viewer.rs | 9 +++-- crates/project/src/image_store.rs | 50 +++++++++++-------------- crates/worktree/src/worktree.rs | 11 +++++- 5 files changed, 39 insertions(+), 33 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0cea2e3ecc..ac9b127ca5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7946,6 +7946,7 @@ dependencies = [ "editor", "file_icons", "gpui", + "language", "log", "project", "schemars", diff --git a/crates/image_viewer/Cargo.toml b/crates/image_viewer/Cargo.toml index 309de4a25f..254c916789 100644 --- a/crates/image_viewer/Cargo.toml +++ b/crates/image_viewer/Cargo.toml @@ -21,6 +21,7 @@ db.workspace = true editor.workspace = true file_icons.workspace = true gpui.workspace = true +language.workspace = true log.workspace = true project.workspace = true schemars.workspace = true diff --git a/crates/image_viewer/src/image_viewer.rs b/crates/image_viewer/src/image_viewer.rs index 9ac8358a17..43ab47c2f4 100644 --- a/crates/image_viewer/src/image_viewer.rs +++ b/crates/image_viewer/src/image_viewer.rs @@ -11,6 +11,7 @@ use gpui::{ InteractiveElement, IntoElement, ObjectFit, ParentElement, Render, Styled, Task, WeakEntity, Window, canvas, div, fill, img, opaque_grey, point, size, }; +use language::File as _; use persistence::IMAGE_VIEWER; use project::{ImageItem, Project, ProjectPath, image_store::ImageItemEvent}; use settings::Settings; @@ -104,7 +105,7 @@ impl Item for ImageView { } fn tab_tooltip_text(&self, cx: &App) -> Option { - let abs_path = self.image_item.read(cx).file.as_local()?.abs_path(cx); + let abs_path = self.image_item.read(cx).abs_path(cx)?; let file_path = abs_path.compact().to_string_lossy().to_string(); Some(file_path.into()) } @@ -149,10 +150,10 @@ impl Item for ImageView { } fn tab_icon(&self, _: &Window, cx: &App) -> Option { - let path = self.image_item.read(cx).path(); + let path = self.image_item.read(cx).abs_path(cx)?; ItemSettings::get_global(cx) .file_icons - .then(|| FileIcons::get_icon(path, cx)) + .then(|| FileIcons::get_icon(&path, cx)) .flatten() .map(Icon::from_path) } @@ -274,7 +275,7 @@ impl SerializableItem for ImageView { cx: &mut Context, ) -> Option>> { let workspace_id = workspace.database_id()?; - let image_path = self.image_item.read(cx).file.as_local()?.abs_path(cx); + let image_path = self.image_item.read(cx).abs_path(cx)?; Some(cx.background_spawn({ async move { diff --git a/crates/project/src/image_store.rs b/crates/project/src/image_store.rs index 22861488bf..205e2ae20c 100644 --- a/crates/project/src/image_store.rs +++ b/crates/project/src/image_store.rs @@ -12,10 +12,10 @@ pub use image::ImageFormat; use image::{ExtendedColorType, GenericImageView, ImageReader}; use language::{DiskState, File}; use rpc::{AnyProtoClient, ErrorExt as _}; -use std::ffi::OsStr; use std::num::NonZeroU64; use std::path::Path; use std::sync::Arc; +use std::{ffi::OsStr, path::PathBuf}; use util::ResultExt; use worktree::{LoadedBinaryFile, PathChange, Worktree}; @@ -96,7 +96,7 @@ impl ImageColorInfo { pub struct ImageItem { pub id: ImageId, - pub file: Arc, + pub file: Arc, pub image: Arc, reload_task: Option>, pub image_metadata: Option, @@ -109,22 +109,11 @@ impl ImageItem { cx: &mut AsyncApp, ) -> Result { let (fs, image_path) = cx.update(|cx| { - let project_path = image.read(cx).project_path(cx); - - let worktree = project - .read(cx) - .worktree_for_id(project_path.worktree_id, cx) - .ok_or_else(|| anyhow!("worktree not found"))?; - let worktree_root = worktree.read(cx).abs_path(); - let image_path = image.read(cx).path(); - let image_path = if image_path.is_absolute() { - image_path.to_path_buf() - } else { - worktree_root.join(image_path) - }; - let fs = project.read(cx).fs().clone(); - + let image_path = image + .read(cx) + .abs_path(cx) + .context("absolutizing image file path")?; anyhow::Ok((fs, image_path)) })??; @@ -157,14 +146,14 @@ impl ImageItem { } } - pub fn path(&self) -> &Arc { - self.file.path() + pub fn abs_path(&self, cx: &App) -> Option { + Some(self.file.as_local()?.abs_path(cx)) } - fn file_updated(&mut self, new_file: Arc, cx: &mut Context) { + fn file_updated(&mut self, new_file: Arc, cx: &mut Context) { let mut file_changed = false; - let old_file = self.file.as_ref(); + let old_file = &self.file; if new_file.path() != old_file.path() { file_changed = true; } @@ -251,7 +240,7 @@ impl ProjectItem for ImageItem { } fn entry_id(&self, _: &App) -> Option { - worktree::File::from_dyn(Some(&self.file))?.entry_id + self.file.entry_id } fn project_path(&self, cx: &App) -> Option { @@ -387,6 +376,12 @@ impl ImageStore { entry.insert(rx.clone()); let project_path = project_path.clone(); + // TODO kb this is causing another error, and we also pass a worktree nearby — seems ok to pass "" here? + // let image_path = worktree + // .read(cx) + // .absolutize(&project_path.path) + // .map(Arc::from) + // .unwrap_or_else(|_| project_path.path.clone()); let load_image = self .state .open_image(project_path.path.clone(), worktree, cx); @@ -604,9 +599,7 @@ impl LocalImageStore { }; image.update(cx, |image, cx| { - let Some(old_file) = worktree::File::from_dyn(Some(&image.file)) else { - return; - }; + let old_file = &image.file; if old_file.worktree != *worktree { return; } @@ -639,7 +632,7 @@ impl LocalImageStore { } }; - if new_file == *old_file { + if new_file == **old_file { return; } @@ -672,9 +665,10 @@ impl LocalImageStore { } fn image_changed_file(&mut self, image: Entity, cx: &mut App) -> Option<()> { - let file = worktree::File::from_dyn(Some(&image.read(cx).file))?; + let image = image.read(cx); + let file = &image.file; - let image_id = image.read(cx).id; + let image_id = image.id; if let Some(entry_id) = file.entry_id { match self.local_image_ids_by_entry_id.get(&entry_id) { Some(_) => { diff --git a/crates/worktree/src/worktree.rs b/crates/worktree/src/worktree.rs index bf3b9e6a26..4d9d1e0196 100644 --- a/crates/worktree/src/worktree.rs +++ b/crates/worktree/src/worktree.rs @@ -107,6 +107,15 @@ pub struct LoadedBinaryFile { pub content: Vec, } +impl fmt::Debug for LoadedBinaryFile { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("LoadedBinaryFile") + .field("file", &self.file) + .field("content_bytes", &self.content.len()) + .finish() + } +} + pub struct LocalWorktree { snapshot: LocalSnapshot, scan_requests_tx: channel::Sender, @@ -3293,7 +3302,7 @@ impl fmt::Debug for Snapshot { } } -#[derive(Clone, PartialEq)] +#[derive(Debug, Clone, PartialEq)] pub struct File { pub worktree: Entity, pub path: Arc,