fluxxcode / egui-file-dialog

Full featured and customizable file dialog for egui
MIT License
91 stars 14 forks source link

show network drives on macOS #194

Closed hacknus closed 6 days ago

hacknus commented 1 week ago

this addresses https://github.com/fluxxcode/egui-file-dialog/issues/168 for macOS.

It still shows my removable drive twice, for some reason, so this bug is still there. But network drives are available and Macintosh HDD is not shown twice anymore.

Bildschirmfoto 2024-11-18 um 18 29 27

Figure: "Space" is a network drive mounted via SMB

EDIT: messed up the rebase on the other PR, so closed it and opened up this one

fluxxcode commented 1 week ago

Could you explain a little bit how you load the network drivers on MacOS? Can't test the changes on MacOS and rely on it being well implemented here.

I actually thought that like in Linux on MacOS all volumes are files and are therefore included in sysinfo::Disksm

hacknus commented 1 week ago

No, in macOS they do not appear and Macintosh HD appears twice, see below (once as / and once as /System/Volumes/Data

Bildschirmfoto 2024-11-18 um 20 01 29 Bildschirmfoto 2024-11-18 um 20 01 35
fluxxcode commented 1 week ago

I think you are the first person to test the file dialog on Mac. Is it completely working there? Can I remove the warning that the dialog has only been tested on Windows and Linux from the readme?

Will merge the PR tomorrow.

hacknus commented 1 week ago

Yes, works fine, really happy that I found this!

Though, there is the weird thing with the volumes (some removable drives appear twice with a slightly different name) with this PR it will also show the time-machine drive (system built-in backup drive, which is invisible normally, we could set it to not show invisible drives in this PR, which might make sense[?]...) There are some minor improvements that could be made:

I could open up issues with these two points, they do not need to be fixed with this PR.

hacknus commented 1 week ago

Would look like this without the invisible drives:

if let Ok(entries) = fs::read_dir("/Volumes") {
    for entry in entries.filter_map(Result::ok) {
        let path = entry.path();
        if seen_mount_points.insert(path.clone()) {
            if let Some(name_osstr) = path.file_name() {
                if let Some(name) = name_osstr.to_str() {
                    if path.is_dir() && !name.starts_with('.') {
                        result.push(Disk::from_path(&path, canonicalize_paths));
                    }
                }
            }
        }
    }
}

managed to take out the metadata here as well.

Should I commit this as well?

fluxxcode commented 1 week ago

the home folder on macOS is usually just the name of the user, so it might be confusing to have it named "home"

Ye, the same applies to Windows, but I think it's not so important that we need to change it. Usually everyone understands what the home directory means, and changing the name per platform is just too much hassle when supporting multilingual ^^

the pinned folders are set to defaults, it might be nice to have the option to read out the plist and set it up the same as in the Finder, so that the user experience is more similar

You can already add custom sections to the left sidebar using FileDialog::add_quick_access (or something similar) from the backend. Additionally, users can already right-click a directory and pin it to the left sidebar. Does that help?

Should I commit this as well?

Yes, of course, that’s part of the PR ;)

hacknus commented 1 week ago

Yes, I found the pin function already, have to check out the quick_access, thanks! What I noticed is that pinned folders get removed if they are unavailable. In my case, I have a pinned folder from a network drive. But I also use my app when the folder is not available, it would be nice if it would appear again, once the drive is mounted again. But I see that this is a rather niche issue...

fluxxcode commented 1 week ago

Have you thought about storing the pinned folders persistently with FileDialogStorage?

hacknus commented 1 week ago

ah yes, perfect, that works!

fluxxcode commented 1 week ago

@hacknus is the PR finished now?

hacknus commented 1 week ago

Yes!

fluxxcode commented 6 days ago

Thanks for implementing!