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

Add file format popup mac #1847

Merged
merged 8 commits into from
Aug 13, 2021
Merged

Conversation

terhechte
Copy link
Contributor

This fixes #998 by adding a file format popup. A couple of caveats (as also explained in my comment here:

  • MacOS usually shows these for NSDocument based apps. In order to have a similar experience, we have to build our own view hierarchy. This might not be pixel perfect similar to, say, how TextEdit does it.
  • The implementation does not return the selected save path and the selected file format. Instead, the save path will be mutated based on the selected file format. So if the user enters "example.pdf" as the path and set set file format to "Jpg" the result will be that the path returned from calling the save panel is "example.jpg".
  • The string File Format: is currently hard coded into the implementation. This is fine but will break with localization. There's no way to retrieve the operating system resource that is used in apps like TextEdit.

This is what it looks like:
Screen Shot 2021-07-02 at 11 41 21

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.

Cool, this looks good! A few comments inline, and CI is complaining in a few places, but all trivial things. Thanks!

druid-shell/src/dialog.rs Outdated Show resolved Hide resolved
druid-shell/src/dialog.rs Outdated Show resolved Hide resolved
@@ -142,3 +164,116 @@ pub(crate) unsafe fn build_panel(ty: FileDialogType, mut options: FileDialogOpti
}
panel
}

// AppKit has a build-in file format accessory view. However, this is only
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// AppKit has a build-in file format accessory view. However, this is only
// AppKit has a built-in file format accessory view. However, this is only

let _: () = msg_send![label, setDrawsBackground:false];
// FIXME: As we have to roll our own view hierachy, we're not getting a translated
// title here. So we ought to find a way to translate this.
let title = NSString::alloc(nil).init_str("File Format:").autorelease();
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting. our architecture does not expect druid-shell to ever need to be locale aware; the idea is that localization will happen higher up in the stack, with correctly localized strings passed in to druid-shell where needed. I'm not sure how best to do that here, though. For the time being I'm happy to punt on this, but we will want to revisit it eventually..

druid-shell/src/dialog.rs Outdated Show resolved Hide resolved
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.

Great, thanks!

It looks like we used to expose the path field through a getter, but I think having both of these be pub is reasonable.

@cmyr
Copy link
Member

cmyr commented Jul 26, 2021

Okay, CI is failing because FileInfo is created in the other backends, and they aren't setting the new field. Other than that this looks ready to merge. #1846 should merge without any problem 🤞, so I think once that's merged you'll be a "contributor" and CI will run automatically on this when you make changes; if it doesn't just ping me or someone else in order to click the "run" button. 😅

@terhechte
Copy link
Contributor Author

Thanks! @cmyr I'll have a look now. Sorry for the long delay, the last couple of days were rather busy.

@cmyr
Copy link
Member

cmyr commented Aug 4, 2021

I think this needs to be rebased to fix the clippy lints. :(

@terhechte terhechte force-pushed the add-file-format-popup-mac branch from afd0ba2 to 702a131 Compare August 13, 2021 12:10
@terhechte
Copy link
Contributor Author

@cmyr I did a rebase, cargo clippy runs fine on my system. I hope this can be merged soon :-)

I guess there is no easy way to run the Windows test suite locally except by booting up a Windows machine or running a VM, right?

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.

Great, thank you very much @terhechte!

@cmyr cmyr merged commit d9728a4 into linebender:master Aug 13, 2021
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.

macOS: Support choosing file formats in the file dialog.
2 participants