Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Metadata/Information display for files (and preview) #184

Merged
merged 92 commits into from
Dec 5, 2024

Conversation

hacknus
Copy link
Contributor

@hacknus hacknus commented Nov 10, 2024

Thank you very much for the work on this great project!
I'm currently implementing preview functionality for this crate.

It is still WIP, I'm mostly doing this for a custom file preview implementation for my project, so I'm not sure how much I will be pushing this further on (depends a bit on the responses here).

I currently implemented an image preview and a text-preview for text files, csv, etc...

The image preview is very slow in dev mode but in release mode, it is almost not noticeable on my machine (Apple M2, SSD), not sure how this could be improved.
Overall, this is a rather 'heavy' thing, so it would probably be best to have it as a feature for the crate, not enabled per default.

Bildschirmfoto 2024-11-10 um 20 53 41

@hacknus hacknus marked this pull request as draft November 10, 2024 19:55
@fluxxcode
Copy link
Owner

Hi, thanks for the kind words and your interest in the project!

With the PR #170 it is now already possible to render your own ui in the right panel without having to change anything in the file dialog code. This would allow you to actually implement the previews directly in your crate instead of a fork.

However, I think it is a good idea if the file dialog already shows a default UI with file information in the right panel and possibly also an image preview.

I've already skimmed through your code and here are some first thoughts:

  • The metadata of an item should be stored directly in the DirectoryEntry object (except the loaded image and data that takes long to load) instead of a single variable in the file dialog. We will need the metadata such as file size in other places as well, e.g. for the issue: Option to display file sizes and modification dates, and optionally sort by them #165. Therefore, the data should be loaded for each item in the directory, instead of just one.
  • Yes, the image preview should be an optional feature. But I think that with the image loaders we don't have to do much here. We can simply use ui.image in egui and then the app is responsible for implementing the image loaders. But we shouldn't actually need a separate dependency for this. (But an option in FileDialogConfig, for example show_image_previews)
  • It should be as easy as possible to extend the supported image previews. However, this should also already be implemented by the image loaders
  • I think it is cleanest if we implement the file preview in a separate module and struct

Let me know when you have continued working on the PR and I should take another look at it. Overall, I think the idea is great :)

@hacknus
Copy link
Contributor Author

hacknus commented Nov 11, 2024

awesome, thanks a lot for looking at this!
update_with_custom_right_panel from #170 certainly makes it a lot easier.
I will look into how we save the metadata in DirectoryEntry. As for the images: the loaders need to be called in the main file (so the user has to do this) and it requires two different dependencies, but with a feature flag, this can easily be done.

egui_extras = { version = "0.29", features = ["all_loaders"] }
image = { version = "0.25", default-features = false, features = ["png", "jpeg"] }

I'm currently thinking of implementing a function information_panel() that becomes available, once the "information" feature is enabled? Then the user could call this with update_with_custom_right_panel. What do you think?

We would probably still need to store the image and file content in the file dialog code or DirectoryEntry, but this can also be feature dependent.

@fluxxcode
Copy link
Owner

I would use the methods completely internally. So I would do it like this: by default the file dialog itself calls the method information_panel() in the right panel and thus displays the information of a file by default. With the update_with_right_panel_ui method the user can then override the behavior and render a custom UI in the right panel. I would not expose the information_panel() method, the problem is that 90% of people don't read the documentation and then don't know that such a thing exists 🙈

But there should then be the config option FileDialogConfig::show_right_panel to completely disable the default information panel.

@fluxxcode
Copy link
Owner

Mhh, I think I'll change my mind and an exposed information_panel might be useful.

We could solve this by default, if enabled, the information_panel method will be used with the normal update method of the dialog. If this method is then exposed, it allows you to use this method also with update_with_right_panel_ui and further add a custom UI next to the default information panel.

@hacknus
Copy link
Contributor Author

hacknus commented Nov 11, 2024

What do you think of this:

 self.file_dialog.update_with_right_panel_ui(ctx, &mut Information_panel::new().ui);

then one could change things in this struct, by for example:

 self.file_dialog.update_with_right_panel_ui(ctx, &mut Information_panel::new().show_file_modified().ui);

I'm not sure how we could add the possibility for users to easily add other file-type readers / displays.
I think that it would be nice that all standard things are shown by default, but if someone wants to add a different preview - for example a "renderer" for a csv, that this can be implemented. But this could also be too difficult and it would be maybe more suitable to provide an example for this information panel and people can then just copy it and extend it?

@fluxxcode
Copy link
Owner

fluxxcode commented Nov 11, 2024

I like the idea. A preview of custom file types can then be implemented relatively easily as follows:

self.file_dialog.update_with_right_panel_ui(ctx, |ui, dialog| {
    // Show custom preview for unsupported file formats
    if dialog.active_entry().path().extension() == "csv" { 
        ui.label("My custom preview for csv files!");
    }

    // Show default file information below the custom preview
    InformationPanel::new().ui(ui, dialog);
});

I think this is the easiest and most flexible way.

Quick note: The panel should be named in Camel Case: InformationPanel and it is best if we put it in its own file/module :)

@hacknus
Copy link
Contributor Author

hacknus commented Nov 11, 2024

Ah yeah, sorry, I first had the Panel as a function, InformationPanel is of course the way to go!

I see an issue with the implementation above:
The information panel has a title "Information" which will then be below this custom implementation. But I think we can do something like this:

self.file_dialog.update_with_right_panel_ui(ctx, |ui, dialog| {
    // Show default file information below the custom preview
    InformationPanel::new().add_file_preview("csv", |ui| {
        ui.label("My custom preview for csv files!");}
    ).ui(ui, dialog);
});

This would also allow overriding existing previews.

@fluxxcode
Copy link
Owner

That's a good idea! Then we can implement default file previews but also still offer the option of overwriting the defaults.

@hacknus
Copy link
Contributor Author

hacknus commented Nov 11, 2024

But I think the constructor should be called in the constructor of the app, and not inside the loop?

struct MyApp {
    file_dialog: FileDialog,
    information_panel: InformationPanel,
    selected_file: Option<PathBuf>,
}

impl MyApp {
    pub fn new(_cc: &eframe::CreationContext) -> Self {
        Self {
            file_dialog: FileDialog::new(),
            information_panel: InformationPanel::new()
                        .add_file_preview("csv", |ui| {
                            ui.label("my csv preview.");
                        }),
            selected_file: None,
        }
    }
}

impl eframe::App for MyApp {
    fn update(&mut self, ctx: &egui::Context, _frame: &mut eframe::Frame) {
        egui::CentralPanel::default().show(ctx, |ui| {
            if ui.button("Select file").clicked() {
                self.file_dialog.select_file();
            }

            self.file_dialog.update_with_right_panel_ui(ctx, &mut |ui, dia| {
                    self.information_panel.ui(ui, dia);
            });

            ui.label(format!("Selected file: {:?}", self.selected_file));
        });
    }
}

fn main() -> eframe::Result<()> {
    eframe::run_native(
        "File dialog example",
        eframe::NativeOptions::default(),
        Box::new(|ctx| {
            egui_extras::install_image_loaders(&ctx.egui_ctx);
            Ok(Box::new(MyApp::new(ctx)))
        }),
    )
}

@fluxxcode
Copy link
Owner

Yep, that's probably better. But the app that uses the info panel can decide for itself. Shouldn't change anything in the implementation of the feature :)

@hacknus
Copy link
Contributor Author

hacknus commented Nov 11, 2024

Okay, I reverted the changes from before and came up with a structure. Still a lot of TODOs.

  • text rendering
  •  image rendering
  • load and show metadata (DirectoryEntry)

The 'renderers' are now set in a HashMap:

pub fn add_file_preview(
        mut self,
        extension: &str,
        add_contents: impl FnMut(&mut Ui, Option<String>, Option<RgbaImage>) + 'static,
    ) -> Self {
        self.supported_files
            .insert(extension.to_string(), Box::new(add_contents));
        self
    }

and can have access text (file content) and image (preview image). Not sure if the thumbnail-image can not be extracted from the metadata though... We will see if this is still needed later.

    information_panel: InformationPanel::new().add_file_preview(
              "csv",
              |ui, text, image| {
                  ui.label("CSV preview:");
                  if let Some(content) = text {
                      egui::ScrollArea::vertical()
                          .max_height(100.0)
                          .show(ui, |ui| {
                              ui.add(
                                  egui::TextEdit::multiline(&mut content.clone()).code_editor(),
                              );
                          });
                  }
              },
          ),

Changes to file_dialog.rs:

  • made selected_item: Option<DirectoryEntry> public
  • introduces function to set width of the right side panel: fn set_right_panel_width(&mut self, width: f32) and fn clear_right_panel_width(&mut self) with minor adaptations in update_ui(

@fluxxcode
Copy link
Owner

Thanks in advance for implementing this!

Here are already a few points that caught my attention:

  • Both config and selected_item should not be made public. There are already access methods for both of them: FileDialog::config and FileDialog::active_entry (Note: active entry is going to be renamed in the future, see: Come up with clear and consistent naming for "currently selected" vs "confirmed picked" items #171)
  • With add_file_preview we do not need to pass the preview image. The preview image can be loaded directly via the path of the currently active item. E.g. ui.image(file_dialog.active_entry.unwrap.path()) Then we don't have to save a separate copy of it and caching should already be done by the image loaders.
  • We need to think about how to do this with previews of text files. The problem currently is that we cannot load the entire file into RAM at once. We don’t know how big the file is and we want to avoid loading multiple gigabytes from a file. I think the information panel itself shouldn't provide the data of the files, but that it is implemented by the app instead. This allows the app developer to decide how much of a file is being loaded for the preview. However, the app must somehow ensure that the data is cached and not loaded every frame.
  • We need to think about what we display in the info view when the file dialog is in DialogMode:: SelectMultiple mode.

This is a really big feature, thank you for taking the time to do this work :)

@hacknus
Copy link
Contributor Author

hacknus commented Nov 12, 2024

Thanks for the feedback!

  • used your methods
  • only storing and displaying 1000 characters of textfiles
  • introduced the feature info_panel
  • stuck with the image. when using the loader as you proposed, it gets super slow, because it loads the image at each call in the loop, so I think we are back to storing it somewhere..
ui.image(format!("file://{}",active_entry.as_path().display()));
  • if multiple files are selected, the last clicked is shown, I think that works out, what do you think?
  • still need to implement the metadata loading in DirectoryEntry

@fluxxcode
Copy link
Owner

But that shouldn't be the case with the images. According to the documentation (https://docs.rs/egui/latest/egui/load/index.html) and my experience, the images are only loaded once and then automatically stored inside the context using the image loaders.

If the performance is not good, something else is wrong. I have to try it out myself later today

@hacknus
Copy link
Contributor Author

hacknus commented Nov 12, 2024

Yeah, I see that, I'm also puzzled why the images do not seem to be cached... maybe I need to profile it to see where it uses all the resources..

@hacknus
Copy link
Contributor Author

hacknus commented Nov 13, 2024

flamegraph
image handling seems to be most of the work done. To be fair - I'm previewing a 16MB file here. And it does indeed get faster after the first load, but it is still not as responsive as I would like to have it.

I think we should focus on opening the thumbnail from the metadata (exif, for JPEG). Not sure how to handle that with png - maybe it is just the way that this will be slower... Do you have an input on that?

@fluxxcode
Copy link
Owner

What do you mean by opening the thumbnail from the exif data?

Are you able to share your test code including the image you are trying to display? Then I can try it out for myself and see what the current performance is like. 16 MB image files are or larger are relatively common, so we should try to get the performance as good as possible.

I'm just thinking about it... Maybe at some point we want to show preview images in the central view. Then we would need that in addition to the meta information for each image in the directory 🤔

@hacknus
Copy link
Contributor Author

hacknus commented Nov 13, 2024

to my knowledge, JPEG files contain a thumbnail in the EXIF. This allows to read only the metadata and not the entire file, and then subsequently resizing it to display the preview. For PNGs we would still have to do that though...

My test code is as is on my branch, but with lines 41-68 in information_panel.rs replaced with:

supported_files.insert(
            "png".to_string(),
            Box::new(
                |ui: &mut Ui, text: Option<String>, image: Option<RgbaImage>, item: &DirectoryEntry| {
                    ui.label("Image");
                    ui.image(format!("file://{}", item.as_path().display()));
                },
            ) as Box<dyn FnMut(&mut Ui, Option<String>, Option<RgbaImage>, &DirectoryEntry)>,

The test image is in the root directory: test_image.png

@fluxxcode
Copy link
Owner

fluxxcode commented Nov 14, 2024

I now found the time to test the InformationPanel. Generally speaking, the file dialog is hardly useable as soon as you click on the image.

However, the problem is not with the ui.image but with:

                    let image = if self.load_image_content {
                        image::open(path)
                            .map(|img| {
                                let new_width = 100;
                                let (original_width, original_height) = img.dimensions();
                                let new_height = (original_height as f32 * new_width as f32
                                    / original_width as f32)
                                    as u32;

                                // Resize the image to the new dimensions (100px width)
                                let img = img.resize(
                                    new_width,
                                    new_height,
                                    image::imageops::FilterType::Lanczos3,
                                );
                                img.into_rgba8()
                            })
                            .ok()
                    } else {
                        None
                    };

in InformationPanel::ui because here the image is loaded every frame, for every supported file. If this part is removed, there will be no noticeable difference in performance and the image will also be cached by the image loader as expected.

However, with such a large image there is another problem: It seems as if the file image loader does not load the image async. This means that the file dialog blocks when an image is clicked for the first time. A spinner is briefly displayed, but even that is blocked.

But I think this is exclusively a problem with the file image loader. With the http image loader the image is loaded async and a spinner is displayed.

@hacknus
Copy link
Contributor Author

hacknus commented Dec 5, 2024

Yep, this works!
I think it's fine if we do not clear the memory when we switch between directories.
The huge memory usage is probably an issue of the egui-loader. I'll look into it and create an issue over there. (see here: emilk/egui#5439)
From my side that's it, you can do a final review and merge.

examples/README.md Show resolved Hide resolved
examples/select_file_with_information_view.rs Outdated Show resolved Hide resolved
src/information_panel.rs Outdated Show resolved Hide resolved
src/information_panel.rs Outdated Show resolved Hide resolved
src/information_panel.rs Outdated Show resolved Hide resolved
src/information_panel.rs Show resolved Hide resolved
src/information_panel.rs Outdated Show resolved Hide resolved
src/information_panel.rs Outdated Show resolved Hide resolved
src/information_panel.rs Outdated Show resolved Hide resolved
@fluxxcode
Copy link
Owner

@hacknus is the PR ready for another review?

@hacknus
Copy link
Contributor Author

hacknus commented Dec 5, 2024

@fluxxcode Yes, I'm through now!

Copy link
Owner

@fluxxcode fluxxcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last few cleanup threads

src/information_panel.rs Outdated Show resolved Hide resolved
examples/pick_file_with_information_view.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@fluxxcode
Copy link
Owner

I will now finally merge the PR. Thank you @hacknus for your work and for actively working on it in the last few weeks. Thanks @bircni for the help with the reviews!

@fluxxcode fluxxcode merged commit 300cb6f into fluxxcode:develop Dec 5, 2024
5 checks passed
@fluxxcode fluxxcode mentioned this pull request Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants