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

windows: Add file dialog using IFileOpenDialog #8919

Merged
merged 10 commits into from
Mar 9, 2024
Merged

windows: Add file dialog using IFileOpenDialog #8919

merged 10 commits into from
Mar 9, 2024

Conversation

the-jasoney
Copy link
Contributor

@the-jasoney the-jasoney commented Mar 6, 2024

Release Notes:

  • Added a file dialog for Windows

Copy link

cla-bot bot commented Mar 6, 2024

We require contributors to sign our Contributor License Agreement, and we don't have @the-jasoney 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'.

@the-jasoney
Copy link
Contributor Author

@cla-bot check

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Mar 6, 2024
Copy link

cla-bot bot commented Mar 6, 2024

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

@the-jasoney the-jasoney marked this pull request as draft March 6, 2024 01:13
@the-jasoney the-jasoney marked this pull request as ready for review March 6, 2024 01:16
@the-jasoney the-jasoney changed the title Use IFileOpenDialog to implement prompt_for_paths in windows Added file dialog using IFileOpenDialog Mar 6, 2024
@mikayla-maki mikayla-maki self-assigned this Mar 6, 2024
@SomeoneToIgnore SomeoneToIgnore changed the title Added file dialog using IFileOpenDialog [Windows] Added file dialog using IFileOpenDialog Mar 6, 2024
crates/gpui/src/platform/windows/platform.rs Outdated Show resolved Hide resolved
crates/gpui/src/platform/windows/platform.rs Outdated Show resolved Hide resolved
crates/gpui/src/platform/windows/platform.rs Outdated Show resolved Hide resolved
crates/gpui/src/platform/windows/platform.rs Outdated Show resolved Hide resolved
crates/gpui/src/platform/windows/platform.rs Outdated Show resolved Hide resolved
@mikayla-maki
Copy link
Contributor

Could you include a screenshot of the working file dialog as well?

@the-jasoney
Copy link
Contributor Author

Screenshot 2024-03-06 124312

@mikayla-maki
Copy link
Contributor

I notice that this part of another PR initializes COM for the length of the app, through a different API. Could we use that approach instead? Or is there a compelling reason to keep the approach you're using?

@mikayla-maki
Copy link
Contributor

Once the COM initialization is figured out, and the merge conflicts resolved, I think I'd prefer to merge this one as it looks a little nicer.

@the-jasoney
Copy link
Contributor Author

I notice that this part of another PR initializes COM for the length of the app, through a different API. Could we use that approach instead? Or is there a compelling reason to keep the approach you're using?

I'm pretty sure the approach in the PR would work.

@mikayla-maki
Copy link
Contributor

@the-jasoney when the merge conflicts are resolved, I'd be down to merge this :)

@JunkuiZhang
Copy link
Contributor

JunkuiZhang commented Mar 7, 2024

Once the COM initialization is figured out, and the merge conflicts resolved, I think I'd prefer to merge this one as it looks a little nicer.

The CoInitialize stuff can be deleted, by Microsoft:

OleInitialize calls CoInitializeEx internally to initialize the COM library on the current apartment.

And since the COM library is initialized on an apartment only once, CoInitialize using in this PR actually does nothing. See Microsoft's comments:

Typically, the COM library is initialized on an apartment only once. Subsequent calls will succeed, as long as they do not attempt to change the concurrency model of the apartment, but will return S_FALSE.

Edit: In my previous PR #8809 , actually use OleInitialize with IFileOpenDialog, IFileSaveDialog and IDropTarget, and all works fine.

@maxdeviant maxdeviant changed the title [Windows] Added file dialog using IFileOpenDialog windows: Add file dialog using IFileOpenDialog Mar 7, 2024
@mikayla-maki
Copy link
Contributor

Now that #8959 has been merged, we should get this branch up to date with main and remove the COM initialization / tear down. Then we can merge :)

…-industries-main

# Conflicts:
#	.gitignore
#	Cargo.toml
#	crates/gpui/src/platform/windows/platform.rs
@mikayla-maki
Copy link
Contributor

Seems this needs another merge conflict resolution, and a cargo fmt before we can merge it :)

@mikayla-maki mikayla-maki merged commit 456efb5 into zed-industries:main Mar 9, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants