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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reveal file in file explorer #18

Merged
merged 17 commits into from Mar 17, 2023
Merged

Reveal file in file explorer #18

merged 17 commits into from Mar 17, 2023

Conversation

bash
Copy link
Contributor

@bash bash commented Feb 25, 2023

I'm finally done 馃コ

Resolves #17

Name

I chose reveal because that's the name used in macOS's open command, but I'm open to better suggestions :)

Linux

I found a good explanation of Chromium's solution and used that as an inspiration.
If it fails, I'm using your suggested fallback of opening the containing directory with open.

Errors

I didn't want to expose dbus::Error, so I decided to wrap it in std::io::Error instead.

Dependencies

  • dbus
    • I tried using the dbus-send command, but it does not support variant dictionaries which is one of the parameters for the org.freedesktop.portal.OpenURI interface.
    • I originally used the zbus crate, but it brings in a lot of dependencies, so I switched to the dbus crate. This crate depends on the native libdbus library. I hope that's OK :)
    • Nevertheless, I gated the reveal function behind a feature as to not push the dependency on consumers of this crate.
  • url
    • I'm only using the Url::from_file_path function. I took a quick peek at the source code and should you not want the dependency on url I think it should be fairly easy to implement it ourselves.

Tested on

  • Windows 11
  • WSL (Ubuntu 22.04)
  • macOS Ventura 13.0
  • Fedora 37

@Seeker14491
Copy link
Owner

Seeker14491 commented Feb 27, 2023

Looks good, I just made some changes:

  • I had a look at the zbus version on your reveal-v1 branch, and I agree it pulls in way too many dependencies. The size of the opener-bin binary went from 5MB to 8MB, and compile times were much longer. However, I'd also like to avoid extra system dependencies, so I enabled the dbus crate's vendored feature.
  • I removed the feature-gating on the binary. The binary only exists for testing and debugging purposes, so we don't need the feature-gating.

Any thoughts?

edit: I found one little issue which I also encountered in a related PR: On Windows, if you reveal a non-existent file, no error is returned; the explorer just opens the top-level "This PC" view.

@bash
Copy link
Contributor Author

bash commented Feb 27, 2023

I removed the feature-gating on the binary. The binary only exists for testing and debugging purposes, so we don't need the feature-gating.

Makes sense - I wasn't sure about the binary's purpose :)

Regarding the non-existent file handling on Windows: Should I manually check that the file exists?
Of course if the file is deleted between our check and the call to explorer.exe we'll still get the "This PC" view...
I think on Linux and macOS it always returns an error (Although I'm not sure about the org.freedesktop.portal.Desktop portal).

@Seeker14491
Copy link
Owner

I found there is a Windows API function we could use instead, which does return an error code if the file doesn't exist. I think that would be the best solution for Windows. I found this repo which demonstrates that approach (though I think for correctness with respect to COM, it should be using a separate thread).
For WSL though, I don't know of a better approach, so maybe checking if the file exists beforehand is the best we can do.

@bash
Copy link
Contributor Author

bash commented Feb 28, 2023

I have experimented with this function before, but I was worried about the CoInitialize call as the concurrency mode can't be changed after it's been set for a thread. Having a separate thread dedicated for the API call is a really creative solution :)

Should I spawn a thread for each call to reveal or should I keep a thread around that I can re-use?

I'll take a look at the example repostitory, maybe I can incorporate that.

@Seeker14491
Copy link
Owner

Since you linked the Chromium repo above, I was curious how they implemented this on Windows so I looked at the source and that's where I learned about this Windows API function. I also noticed they were using a worker thread. Source link

I think spawning a thread for each call is fine; this is a function that would be called infrequently, so the overhead of spawning a thread is negligible.

@bash
Copy link
Contributor Author

bash commented Mar 11, 2023

The latest version now calls SHOpenFolderAndSelectItems on a worker thread. For WSL I'm still using explorer.exe /select.

Copy link
Owner

@Seeker14491 Seeker14491 left a comment

Choose a reason for hiding this comment

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

Looks good. Just this one other detail and I can merge this.

opener/src/windows/reveal.rs Outdated Show resolved Hide resolved
@Seeker14491 Seeker14491 merged commit 5ddb52d into Seeker14491:master Mar 17, 2023
5 checks passed
@bash bash deleted the reveal branch March 18, 2023 09:53
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.

Reveal file in file explorer
2 participants