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

show network drives on macOS #194

Merged
merged 4 commits into from
Nov 19, 2024

Conversation

hacknus
Copy link
Contributor

@hacknus hacknus commented Nov 18, 2024

this addresses #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
Copy link
Owner

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
Copy link
Contributor Author

hacknus commented Nov 18, 2024

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

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.

Thanks for implementing. Created a few threads :)

src/data/disks.rs Outdated Show resolved Hide resolved
src/data/disks.rs Outdated Show resolved Hide resolved
src/data/disks.rs Outdated Show resolved Hide resolved
@fluxxcode
Copy link
Owner

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
Copy link
Contributor Author

hacknus commented Nov 18, 2024

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:

  • the home folder on macOS is usually just the name of the user, so it might be confusing to have it named "home"
  • 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

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

@hacknus
Copy link
Contributor Author

hacknus commented Nov 18, 2024

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
Copy link
Owner

fluxxcode commented Nov 18, 2024

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
Copy link
Contributor Author

hacknus commented Nov 19, 2024

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
Copy link
Owner

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

@hacknus
Copy link
Contributor Author

hacknus commented Nov 19, 2024

ah yes, perfect, that works!

@fluxxcode
Copy link
Owner

@hacknus is the PR finished now?

@hacknus
Copy link
Contributor Author

hacknus commented Nov 19, 2024

Yes!

@fluxxcode fluxxcode merged commit b0ed7c1 into fluxxcode:develop Nov 19, 2024
5 checks passed
@fluxxcode
Copy link
Owner

Thanks for implementing!

@fluxxcode fluxxcode mentioned this pull request Nov 19, 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.

2 participants