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 more file dialog configuration and macOS support. #960

Closed
wants to merge 12 commits into from

Conversation

xStrom
Copy link
Member

@xStrom xStrom commented May 18, 2020

This PR adds a bunch of stuff to the FileDialogOptions struct.

  • default_name to specify the file name that should be pre-filled.
  • name_label to specify text that should appear next to the file name textbox.
  • title to set the title of the dialog.
  • button_text to set the text on the main save/open button.
  • force_starting_directory to support niche use cases where the app wants to override the OS chosen starting directory in the dialog.

To get things going I started with the macOS implementation. However these are all things that can also be supported on Windows, and most if not all also on GTK.

I updated the select_directories documentation to specify that this means that only folders are being selected. There is no way to select both folders and files on Windows. So for a cross-platform API that doesn't make sense. If there is some use case where you want both folders and files to be selected then that should be a new mode called select_files_and_directories or something and have clear documentation about being available only on macOS (and..?).

I also made some macOS specific changes in relation to the existing API:

  • File packages (directories with known extensions) are now treated as plain directories. File packages are a macOS concept and don't translate in a cross-platform way. This is related to runebender#108 because runebender expects directories to be treated as files when they have an extension, but that's a macOS-only behavior. The cross-platform way to do it would be to enable select_directories.
  • Implemented default_type which will be used e.g. in the save dialog when you don't type in an extension.
  • Fixed the save dialog not showing up at all when select_directories or multi_selection are set.

Miscellaneous:

  • Changed FileSpec to use a non-static lifetime. It works the same way with 'static but now also supports non-static use cases.

Fixes #390.
Fixes #902.

@xStrom xStrom added shell/mac concerns the macOS backend S-needs-review waits for review labels May 18, 2020
@xStrom
Copy link
Member Author

xStrom commented May 19, 2020

I was thinking some more about these macOS packages and if some of it could be surfaced through a new select_packages option. I don't see a clear way forward though.

For opening, this select_packages mode could fall back to select_directories on other platforms. However on macOS it could have a special path where it's a file open dialog with packages treated as files. This would allow the file filters to work on macOS in this mode. Although even in this mode the user could still just choose a plain old file instead of a package and I don't see a way to turn that off in the Apple docs.

The real trouble starts with saving though. On macOS it would work fine in that same packages-as-files mode. However on other platforms you can only save files. This would work if you're creating a new package. Because you can just treat that file location as a directory location instead. However what if you want to overwrite an existing package? Well that's a directory and you can't select it in the save dialog. One workaround would be to save a file called "foo.pkg" in the parent directory of the "foo.pkg" package. However that would require manual typing, the user can't click on the package to get that result.

So for now I haven't added any select_packages mode as I can't see a way to do it in a reasonable way.

I do wonder though, how does runebender deal with overwriting existing packages on Windows right now? Requires manual typing of the name?

@cmyr
Copy link
Member

cmyr commented May 28, 2020

Something we're going to need, eventually, is a common pattern for implementing platform specific behaviour, especially around things like this. One bit of prior art we might look at are the various Ext traits in the stdlib, like OsStringExt, which adds methods to OsString for the current platform?

In runebender, if I have to use cfg to create different sets of flags for different platforms, that's fine for now; I just want to make sure we don't decrease the set of things that can be expressed.

I think on mac that file packages should count as files, not directories, and it's up to the user to use cfg to get things to work cross-platform. I think this just because that's conceptually how the mac treats them; they're single 'things' for the purposes of the UI, and a normal user would never know that they're actually folders on the file system.

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.

Thanks, it's definitely good to start fleshing out more of these options, and sorry for letting this sit for so long.

Some concerns about the lifetimes, and about the unexpected (to the user) default behaviour around file packages; but overall this looks like a solid improvement.

@@ -33,12 +33,17 @@ pub enum FileDialogType {

/// Options for file dialogs.
#[derive(Debug, Clone, Default)]
pub struct FileDialogOptions {
pub struct FileDialogOptions<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

I expect this to cause problems? In particular I think we need to be able to Box this type in order to use it with Command.

I believe this was the rationale behind the 'static lifetimes in FileSpec as well.

Copy link
Member Author

@xStrom xStrom May 28, 2020

Choose a reason for hiding this comment

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

You're right. Lifetimes won't work here. However 'static isn't going to work either, because these need to be able to be generated at runtime.

Could just convert them to String - or maybe some sort of Enum of &'static str and String? Or is there a better standard way? Surely this is a common issue.

Perhaps just a Into<String> is the easiest solution. These structs aren't exactly in the hot path, so we should optimize ergonomics over memory usage.

// set options
// Directories with extensions need to be treated as directories.
// File packages are a macOS concept and don't translate in a cross-platform way.
let () = msg_send![panel, setTreatsFilePackagesAsDirectories: YES];
Copy link
Member

Choose a reason for hiding this comment

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

discussed in a separate comment, I'm not sure about this.

@xStrom
Copy link
Member Author

xStrom commented May 28, 2020

Okay so in terms of packages and what the default should be. I can see the value in user expectations as far as browsing the file dialog goes. Being able to traverse into packages is probably not common.

Treating them as files means though that druid apps need to always check if a OPEN_FILE/SAVE_FILE provided FileInfo is a file or a directory(package).

So me defaulting to treating them as directories was optimizing for less error checking in app code.

I don't have any strong opinions here though, so I'm open to defaulting back to treating them as files and just adding a bunch of documentation warning people that file paths may actually be packages on macOS.

Changing this default back doesn't really help out runebender though. It makes it more difficult for runebender. SHOW_OPEN_PANEL should be used only on macOS. (Alternatively looking for metainfo.plist instead, but the point is that the code isn't universal.) This is also one of the reasons I changed the default, so that apps like runebender could have one cross-platform logic for dealing with these. I do understand though that the macOS experience suffers when treating packages as directories. That's why I mentioned the potential select_packages mode. This mode in theory would help deal with packages using a single code path while utilizing all of macOS strengths. However like I pointed out in a previous post, overwriting existing "packages" on Windows looks really messy, so this mode might be rather difficult to implement. (It's probably possible via shell extensions added to the file dialog on Windows. Less sure about options with GTK.)

@xStrom
Copy link
Member Author

xStrom commented May 29, 2020

Okay this is ready for another look.

  • The lifetime issue is resolved by using Into<String> instead.
  • macOS packages are back to being treated as files by default, but can be optionally treated as directories.
  • Added a bunch of docs around packages and their behavior.

@luleyleo
Copy link
Collaborator

Hm, this is super irritating to me as I've never worked with macOS.

This is how I understood the FileDialogOptions docs:

  • files mode
    • not macOS: all matching files
    • macOS: all matching files or packages
      (entering non-system packages is possible)
  • files mode + packages_as_directories
    • all matching files - same on all platforms, packages don't exist
  • directory mode
    • not macOS: all directories, ignoring filters
    • macOS: all directories or packages matching the filters
      (entering non-system packages is possible)
  • directory mode + packages_as_directories
    • not macOS: all directories
    • macOS: all directories matching the filters, packages don't exist

@xStrom
Copy link
Member Author

xStrom commented May 29, 2020

It's definitely convoluted and you didn't quite get it right. Maybe I should rewrite the behavior as a table similar to what you created here.

First, in files mode, you can enter non-system packages only in the save dialog. Not in the open dialog. I'll at the very least add an extra clarification about this even if I don't rewrite the whole paragraph.

In directory mode, you only get directories. You can't interact with packages at all. The file filters decide which directories lose their directory status and are now considered packages. Packages can't be chosen or traversed into.

In directory mode + packages_as_directories you can select and traverse into all directories, packages don't exist. Filters don't do anything, like on other platforms.

@xStrom
Copy link
Member Author

xStrom commented May 30, 2020

Okay I think I improved the documentation some more. This includes adding a table to describe how packages interact with the various dialogs.

Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

That table is great!

What still bugs me a bit about this is that we now have docs for normal platforms and docs for macOS but what seems the most important for a developer, is docs on how to utilize this to implement open and save actions in a cross platform manner.

Sure, one can now sit down, try to comprehend all of this, try to understand what the hell is wrong with macOS (or all other platforms if you use macOS) and then get started with the actual implementation, but it would be nice if we could offer a more comprehensive guide on this topic.

But I think that should be the topic of a new PR, this one is already a great improvement.

/// then you can change the default behavior via [`packages_as_directories`]
/// to force macOS to behave like other platforms and not give special treatment to packages.
///
/// # macOS
Copy link
Collaborator

Choose a reason for hiding this comment

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

This differentiation between Packages and macOS kind of makes it look as if Packages would be relevant on other platforms as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I moved it all under the macOS header.

/// If the clicked file's extension is different than the first extension of the default type
/// then the returned path does not actually match the path of the file that was clicked on.
/// Clicking on a file doesn't change the base path either. So if a user has traversed into
/// `/Users/Joe/foo/` and then clicks on an existing file `/Users/Joe/old.txt` then the returned
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean /Users/Joe/foo/old.txt instead of /Users/Joe/old.txt?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, what I wrote is correct. The macOS file dialog can have multiple directories open at once. Take a look at this screenshot.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a bit of extra clarification to this section, pointing out that the macOS file dialog can have several directories open at once.

@xStrom
Copy link
Member Author

xStrom commented May 30, 2020

Yes this is not ideal. Generally I would prefer to make things automatically cross-platform so that users wouldn't have to worry about things like this.

I do sympathize with macOS users liking their packages and not wanting to lose that. It is a concept that doesn't really translate in a cross-platform way though. So I personally would use something like a zip file to emulate a package and that would be cross-platform.

For developers I guess the guideline would be to avoid using directories with extensions unless they love #[cfg] guards. If your app doesn't need to interact with directories with extensions, then you don't even really need to know that packages are a thing. Packages can appear as packages for macOS users when they browse around in the file dialog, but they're never relevant for your app.

I added a note at the top of the docs informing users of this cross-platform universal path.

Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

This looks quite good already.

One more nit: The Use the save dialog only for new paths section would also be relevant for Cross-platform compatibility I think. Because there it sounds like directories + extensions is the only macOS specialty, but getting open and save correct seems like a must for calling your app 'cross platform'.

@xStrom
Copy link
Member Author

xStrom commented May 30, 2020

You're right, the docs should be giving better actionable advice. I reworded some of that and offer it as general cross-platform advice.

Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

This looks really good now! I've nothing more to add.

@luleyleo luleyleo added the breaking change pull request breaks user code label Jun 6, 2020
@cmyr
Copy link
Member

cmyr commented Sep 25, 2020

closed in favour of #1257

@cmyr cmyr closed this Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change pull request breaks user code S-needs-review waits for review shell/mac concerns the macOS backend
Projects
None yet
3 participants