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

Respect starting directory for dialogs on Windows #1452

Merged

Conversation

MaximilianKoestler
Copy link
Contributor

Before this change, FileDialogOptions::starting_directory had not been used on Windows and therefore had no influence on the starting directory of a file dialog window.

This has now been fixed.

The use of SHCreateItemFromParsingName makes it mandatory to explicitly link against shell32.lib because this is not done automatically since Rust 1.33.0. See rust-lang/rust#56568 for details.

Before this change, FileDialogOptions::starting_directory had not been
used on Windows and therefore had no influence on the starting directory
of a FileDialog window.

This has now been fixed.

The use of `SHCreateItemFromParsingName` makes it mandatory to
explicitly link against shell32.lib because this is not done
automatically since Rust 1.33.0.

See rust-lang/rust#56568 for details.
@@ -0,0 +1,26 @@
// Copyright 2020 The Druid Authors.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a more standard way to do this would be to add the "shellapi" feature to winapi-rs, rather than adding a build.rs here.

Copy link
Contributor Author

@MaximilianKoestler MaximilianKoestler Dec 13, 2020

Choose a reason for hiding this comment

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

Well... that seems to be a much more reasonable idea than what I have done. It was a bit foolish by me to assume that there is no solution for this in the winapi-crate.

Before I add "shellapi" to the list, do you have an idea if there is a specific meaning to how the features of winapi are ordered and formatted?
When I see a block like this in a Cargo.toml, I always wonder where to add something new and where to put the line breaks afterwards:

features = ["d2d1_1", "dwrite", "winbase", "libloaderapi", "errhandlingapi", "winuser",
            "shellscalingapi", "shobjidl", "combaseapi", "synchapi", "dxgi1_3", "dcomp",
            "d3d11", "dwmapi", "wincon", "fileapi", "processenv", "winbase", "handleapi"]

Copy link
Member

Choose a reason for hiding this comment

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

@MaximilianKoestler while I admire your diligence, I don't think there is a clear ordering. For line breaks, a reasonable guideline is to just keep us at 80 columns. In a perfect world ordering would be alphabetical and fixed by rustfmt, but I think you should just add yours at the end (probably after a newline)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmyr Thanks. I know that I am the type of dev who usually thinks about these kinds of things for one minute too long 😄. I will just go with the smallest delta.

While I am at it: In this project, do you prefer

  • changes after review as a separate commit,
  • changes after review as a separate commit, but squashed during PR merge,
  • or just force pushing on the PR branch?

Copy link
Member

Choose a reason for hiding this comment

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

We are decidedly anarchic. For a small patch, force pushing is fine; at merge time if commits are clearly written or clearly intentional in their structure I will rebase, and if things are more messy I will squash.

As it turns out, the winapi crate already exposes shell32.lib via a
cargo feature, so there is no need to link it manually.
@cmyr
Copy link
Member

cmyr commented Dec 13, 2020

Okay, this looks good, outside of the changelog conflict.

I'm not able to test on windows today but will be able tomorrow, if nobody beats me to it.

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

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

The code change looks good to me. As soon as anyone can verify this works on Windows, I'm happy for it to be merged. (I might be able to do it later myself - is there a quick repro sequence?) Thanks for the contribution and welcome!

@MaximilianKoestler
Copy link
Contributor Author

is there a quick repro sequence?

@raphlinus

diff --git a/druid/examples/open_save.rs b/druid/examples/open_save.rs
index 21d1a34..1a61d0d 100644
--- a/druid/examples/open_save.rs
+++ b/druid/examples/open_save.rs
@@ -42,6 +42,7 @@ fn ui_builder() -> impl Widget<String> {
         .allowed_types(vec![rs, txt, other])
         .default_type(txt)
         .default_name(default_save_name)
+        .force_starting_directory(std::path::Path::new("C:\\"))
         .name_label("Target")
         .title("Choose a target for this lovely file")
         .button_text("Export");

and then

druid\druid> cargo run --example open_save

should be sufficient to verify the behavior. With the added line, the "save" dialog opens at C:\, without it opens at the default location (probably %USERPROFIL%\Documents).

@cmyr
Copy link
Member

cmyr commented Dec 15, 2020

oops forgot this yesterday will take a look when I get in to my work space this afternoon.

Copy link
Member

@cmyr cmyr 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, thanks. Feel free to fixup the changelog or I can do it and squash on my next pass through.

@MaximilianKoestler
Copy link
Contributor Author

@cmyr I resolved the conflicts, feel free to merge & squash whenever you like 😄.

@cmyr cmyr merged commit f4b98bd into linebender:master Dec 16, 2020
@MaximilianKoestler MaximilianKoestler deleted the windows-dialog-starting-directory branch December 17, 2020 14: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.

3 participants