Add image dimension and file size information

[kaf-lamed-beyt]

Closes #21281

@jansol, kindly take a look when you're free.

Release Notes:

• Added dimensions and file size information for images.

[cla-bot]

We require contributors to sign our Contributor License Agreement, and we don't have @kaf-lamed-beyt on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

[kaf-lamed-beyt]

@cla-bot check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[Angelk90]

@kaf-lamed-beyt :
Great job, personally I would do this:

1. I would remove the Dimension: and Image size:
2. I would show this information here below on the right at the beginning of these icons

You can take a screenshot of the image located at the following path: ./crates/zed/resources/app-icon-dev.png

I don't understand why webstorm and vscode give two different weight dimensions.

Vscode:

Webstorm:

[kaf-lamed-beyt]

Alright! Thank you for the feedback @Angelk90. I'll take the screenshot and share it here.

Quick question though... which crate would I need to update to show this image data in the location you asked me to move it? I'm thinking this one: crates/zed/src/zed/app_menus.rs?

Perhaps, using the StatusItemView?

[kaf-lamed-beyt]

Here's what i have

[Angelk90]

@kaf-lamed-beyt : Maybe I figured out why the size of webstorm is different, it is measured using kB and not KB.

kB = 1,000 bytes (Decimal system, preferred in commercial and in the most recent specifications).
KB = 1,024 bytes (Binary system, more traditional in computer science).

But the calculations do not work:
189.48 KB × 1,024 = 194,355.72 bytes
194,355.72 bytes / 1,000 = 194.36 kB

194,360 bytes (webstorm size) − 194,030 bytes = 330 bytes

Anyway, an important point in my opinion is the convention of whether to use a dot or a comma to define decimals.

Webstorm uses a comma, VScode uses a dot from what I see.
I think this is an important thing to define correctly.

To understand where best to put this information I think you need to ask @danilo-leal.

Personally I don't like the file path being visible, if it were possible from the settings I would remove it from mine.

@kaf-lamed-beyt : If you can show this information for the images too that would be great.

[kaf-lamed-beyt]

Oh! Now I understand the reason for the disparity in file size. So what do you recommend we use in this case @Angelk90? the binary or decimal system?

We can decide to accommodate both of them though. If that's okay

@kaf-lamed-beyt : If you can show this information for the images too that would be great.

cool I'll include this too.

[kaf-lamed-beyt]

@danilo-leal, hi 👋🏼

any idea how I can include the image metadata in the status bar?

[Angelk90]

@kaf-lamed-beyt :
I would let the user choose which unit of measurement he prefers, for example in the default settings I would create a key called I don't know for example:

https://github.com/zed-industries/zed/blob/main/assets/settings/default.json

{
  "system_type": "binary"
}

default binary, another possible choice decimal, If a value other than the two or incorrect is defined, use the default one

{
  "decimal_separator": "dot", // dot or comma
}

Same thing as above.

example code:

fn format_size(size: u64, base: f64, units: &[&str], separator: char) -> String {
    let mut size_in_units = size as f64;
    let mut unit_index = 0;

    while size_in_units >= base && unit_index < units.len() - 1 {
        size_in_units /= base;
        unit_index += 1;
    }

    let formatted_size = format!("{:.2}", size_in_units);
    
    let formatted_size = match separator {
        ',' => formatted_size.replace('.', ","),
        _ => formatted_size
    };

    format!("{} {}", formatted_size, units[unit_index])
}

fn format_size_decimal(size: u64, separator: char) -> String {
    let units = ["B", "KB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"];
    format_size(size, 1000.0, &units, separator)
}

fn format_size_binary(size: u64, separator: char) -> String {
    let units = ["B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB", "ZiB", "YiB"];
    format_size(size, 1024.0, &units, separator)
}

We'll have to see what the maintainers think of these choices.

[jansol]

That was fast! Nice work so far!

Anyway, an important point in my opinion is the convention of whether to use a dot or a comma to define decimals.

The decimal separator is defined by the user's locale. We should not try to be clever about it. nvm apparently format! does not do this

any idea how I can include the image metadata in the status bar?

Don't. That won't work well if someone opens multiple images side by side. Also there is very limited space in the status bar and I would very much like to see a lot more metadata (file format; color format - channels, bit depth, subsampling, etc; physical size/DPI; comment; whether the file has color profile; ...)

As far as I've seen, most applications tend to put metadata in an overlay or a side bar. I think @iamnbutler is the one that should be consulted on that, as he is the main designer in the Zed team AIUI. You might want to arrange a collab call with him to decide on the UI design portion of this.

[Angelk90]

@jansol :

The decimal separator is defined by the user's locale.

Where and how?
In the settings where is this setting found?

That won't work well if someone opens multiple images side by side.

Depending on which image the tab focus is on, information is displayed just like vscode does.

However, I agree with the choice of having a side menu for file information, as xcode does.

[kaf-lamed-beyt]

That was fast! Nice work so far!

Thank you, @jansol!

Don't. That won't work well if someone opens multiple images side by side. Also there is very limited space in the status bar and I would very much like to see a lot more metadata (file format; color format - channels, bit depth, subsampling, etc; physical size/DPI; comment; whether the file has color profile; ...)

Oh! That's good then. This is a valid reason.

I'd check if the image crate can provide all these metadata you mentioned.

What's the best way to reach @iamnbutler? Email? Twitter? Discord (I may need the discord handle)

[mikayla-maki]

Personally, I prefer the VSCode style with the size and dimensions in the status bar. You can define a new status bar item in the image_viewer crate, and then add it after the cursor position indicator here:

zed/crates/zed/src/zed.rs

Lines 214 to 222 in d7ea3dd

	workspace.status_bar().update(cx, |status_bar, cx| {
	status_bar.add_left_item(diagnostic_summary, cx);
	status_bar.add_left_item(activity_indicator, cx);
	status_bar.add_right_item(inline_completion_button, cx);
	status_bar.add_right_item(active_buffer_language, cx);
	status_bar.add_right_item(active_toolchain_language, cx);
	status_bar.add_right_item(vim_mode_indicator, cx);
	status_bar.add_right_item(cursor_position, cx);
	});

[mikayla-maki]

Let's elevate this dependency, and the one in GPUI, to a workspace dependency

[Angelk90]

I would very much like to see a lot more metadata (file format; color format - channels, bit depth, subsampling, etc; physical size/DPI; comment; whether the file has color profile; ...)

@mikayla-maki : what @jansol proposed cannot be done.

Let's see what @iamnbutler says.

[kaf-lamed-beyt]

@mikayla-maki am i on the right path?

I've managed to get a UI element to render in the status bar. But, I do not think the load_metadata fn here works as it should. I even tried logging to the console so I could see if the data was retrieved or not.

zed/crates/project/src/image_store.rs

Lines 135 to 157 in b27afc4

	pub async fn load_metadata<F: Fs>(
	&mut self,
	project: &Project,
	cx: &AppContext,
	fs: &F,
	) -> Result<(), String> {
	println!("Loading metadata for image: {:?}", self.path());
	match Self::try_open(self, project, cx, fs).await {
	Ok(metadata) => {
	self.width = Some(metadata.0);
	self.height = Some(metadata.1);
	self.file_size = Some(metadata.2);
	self.color_type = Some(metadata.3);
	println!("Metadata loaded: {:?}", metadata);
	Ok(())
	}
	Err(err) => {
	println!("Failed to load metadata: {}", err);
	Err(err)
	}
	}
	}

Perhaps, you could point me in the right direction?

[iamnbutler]

Hey, sorry I'm terrible at keeping up with activity here.

My first instinct is that the bottom right of the toolbar should be used to show this metadata.

In buffers, that is cursor position, selected lines, characters, etc. So for images it would make sense for it to show file size, dimensions, etc.

There is the downside that @jansol mentions, that you can't do comparisons in this way.

I wouldn't be opposed to us exploring other options such as overlays, but I would not put this info in the toolbar, as it is intended for controls.

If we had sub-splits already (in-buffer sidebars) I would suggest adding a i icon that opens a panel in the buffer where we can show information, similar to finder:

[kaf-lamed-beyt]

Thanks for the feedback @iamnbutler! The approach I'm taking is in line with what you mentioned here

My first instinct is that the bottom right of the toolbar should be used to show this metadata.
In buffers, that is cursor position, selected lines, characters, etc. So for images it would make sense for it to show file size, dimensions, etc.

But, I have difficulty getting the image metadata to show up there. I successfully got a UI element to render in the status(tool)bar:a placeholder.

So any help, reviewing what I'm doing wrong, would be very much appreciated.

If we decide to have a sub-split like you the image description you shared, where or which crates should I be focusing on for this implementation?

This is my first "real" rodeo with Rust, so you'll please excuse my questions 👐🏼. Thanks!

[mikayla-maki]

But, I have difficulty getting the image metadata to show up there.

You'll have to figure it out if you want to ship this PR :). I'd suggest looking at the cursor position UI, and learning how it's hooked into the rest of Zed. The key trait you'll want to understand is StatusItemView, and you can see how it's implemented for the CursorPosition struct here:

zed/crates/go_to_line/src/cursor_position.rs

Lines 212 to 232 in 2ff06ce

	impl StatusItemView for CursorPosition {
	fn set_active_pane_item(
	&mut self,
	active_pane_item: Option<&dyn ItemHandle>,
	cx: &mut ViewContext<Self>,
	) {
	if let Some(editor) = active_pane_item.and_then(|item| item.act_as::<Editor>(cx)) {
	self._observe_active_editor =
	Some(cx.observe(&editor, |cursor_position, editor, cx| {
	Self::update_position(cursor_position, editor, Some(UPDATE_DEBOUNCE), cx)
	}));
	self.update_position(editor, None, cx);
	} else {
	self.position = None;
	self._observe_active_editor = None;
	}
	cx.notify();
	}
	}

I'm going to mark this as a draft for now, feel free to mark it as ready once you have a new implementation utilizing the status bar working.

Good luck and happy holidays!

[kaf-lamed-beyt]

Thank you @mikayla-maki! I've been going through the CursorPosition implementation, actually.

But, I'll go with your suggestion and approach everything carefully now.

I'll make sure to mark this PR as ready when I'm set. Happy holidays!

[kaf-lamed-beyt]

Out of 19 failures previously, now it is 1 failure. Good, good!

@mikayla-maki... the image store test still fails. See the CI output

 Summary [ 137.050s] 1545 tests run: 1544 passed (3 slow), 1 failed, 0 skipped
        FAIL [   0.350s] project image_store::tests::test_image_not_loaded_twice

And here's what init_test looks like:

  pub fn init_test(cx: &mut TestAppContext) {
        if std::env::var("RUST_LOG").is_ok() {
            env_logger::try_init().ok();
        }

        cx.update(|cx| {
            let settings_store = SettingsStore::test(cx);
            cx.set_global(settings_store);
            language::init(cx);
            Project::init_settings(cx);
            image_viewer::init(cx);
        });
    }

[mikayla-maki]

@kaf-lamed-beyt You can run that test locally and see what the error is with cargo. cargo test -p project test_image_not_loaded_twice :)

[kaf-lamed-beyt]

alright, i'll do this now

[kaf-lamed-beyt]

Hi @mikayla-maki, I think it's still the same issue from before

My bad! I used what you suggested now, instead of cargo test -p image_viewer, and here's what i have (which is quite the same):

error[E0425]: cannot find function `open_test_db` in crate `$crate`
   --> crates/image_viewer/src/image_viewer.rs:378:5
    |
378 | /     define_connection! {
379 | |         pub static ref IMAGE_VIEWER: ImageViewerDb<WorkspaceDb> =
380 | |             &[sql!(
381 | |                 CREATE TABLE image_viewers (
...   |
391 | |             )];
392 | |     }
    | |_____^ help: a function with a similar name exists: `open_main_db`
    |
   ::: /oss/zed/crates/db/src/db.rs:76:1
    |
76  |   async fn open_main_db<M: Migrator>(db_path: &Path) -> Option<ThreadSafeConnection<M>...
    |   ------------------------------------------------------------------------------------- similarly named function `open_main_db` defined here
    |
note: found an item that was configured out
   --> /oss/zed/crates/db/src/db.rs:99:14
    |
99  | pub async fn open_test_db<M: Migrator>(db_name: &str) -> ThreadSaf...
    |              ^^^^^^^^^^^^
note: the item is gated here
   --> /oss/zed/crates/db/src/db.rs:98:1
    |
98  | #[cfg(any(test, feature = "test-support"))]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: this error originates in the macro `define_connection` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0425`.
error: could not compile `image_viewer` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...

[kaf-lamed-beyt]

So @mikayla-maki, two things; I realized — after looking at the error closely — that the open_test_db needs to work in a "test-support" scope, which was missing in the image_viewer crate, so i included it in Cargo.toml and ran the (cargo test -p image_viewer) test (which was failing before) again but this time around with the --features test-support — flag and it passed.

I pushed a commit just now, so i think the CI should pass now @iamnbutler

But, doing this:

cargo test -p project test_image_not_loaded_twice --features test-support

led to this:

    Finished `test` profile [unoptimized + debuginfo] target(s) in 3.08s
     Running unittests src/project.rs (target/debug/deps/project-71c130b4fc529ae4)

running 1 test
test image_store::tests::test_image_not_loaded_twice ... FAILED

failures:

---- image_store::tests::test_image_not_loaded_twice stdout ----
thread 'image_store::tests::test_image_not_loaded_twice' panicked at crates/project/src/image_store.rs:830:34:
called `Result::unwrap()` on an `Err` value: File does not exist at path: "/root/image_1.png"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    image_store::tests::test_image_not_loaded_twice

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 81 filtered out; finished in 0.77s

error: test failed, to rerun pass `-p project --lib`

dunno if it is expected or not.

[kaf-lamed-beyt]

@iamnbutler, same error from the CI

──── STDERR:             project image_store::tests::test_image_not_loaded_twice
thread 'image_store::tests::test_image_not_loaded_twice' panicked at crates/project/src/image_store.rs:830:34:
called `Result::unwrap()` on an `Err` value: File does not exist at path: "/root/image_1.png"

I don't think I need to have /root/image_1.png, do I?