Implemented multiple selection for gtk backend #2081
Conversation
| format: None, | ||
| // If result is an error, return an empty vector, | ||
| // otherwise return a vec of FileInfos | ||
| .map_or(vec![], |f| { |
There was a problem hiding this comment.
I know this is the same behavior as before, but it does seem like the errors from get_file_dialog_path should get logged, at least...
There was a problem hiding this comment.
Makes sense, I'll add a log here
druid/src/win_handler.rs
Outdated
| @@ -80,6 +80,8 @@ struct DialogInfo { | |||
| id: WindowId, | |||
| /// The command to send if the dialog is accepted. | |||
| accept_cmd: Selector<FileInfo>, | |||
| /// The command to send if the dialog is accepted with multiple files. | |||
| accept_multiple_cmd: Option<Selector<Vec<FileInfo>>>, | |||
There was a problem hiding this comment.
Is it necessary for this to be an Option? I mean, I see that it's not used for save dialogs, but then save dialogs shouldn't get configured for multi-selection anyway...
There was a problem hiding this comment.
It wasn't an Option in my first implementation, I made it so to avoid having the field in save dialogs as you noticed.
If you think this isn't a problem though, having it always set makes the code nicer elsewhere, so I'm going to change this.
| filenames if filenames.is_empty() => Err(anyhow!("No path received for filename")), | ||
| // This really shouldn't happen, unless there is a bug | ||
| // when setting the dialog options | ||
| filenames if filenames.len() > 1 && !options.multi_selection => { |
There was a problem hiding this comment.
One last corner case that I noticed because of the unwrap below: if they pass FileDialogType::Save and options.multi_selection = true (which they shouldn't but I don't think we prevent it), then we correctly handle that combination on line 62 (by ignoring multi selection) but we don't check here that we only got one file back. Ok, maybe that's a bit paranoid, but I don't think it's any more paranoid than the existing checks that we have here...
There was a problem hiding this comment.
I'm not sure I understand what you mean:
if they pass FileDialogType::Save and options.multi_selection = true, then we correctly handle that combination on line 62 but we don't check here that we only got one file back
We do check that the we only got one file back (filenames.is_empty and filenames.len() > 1), and return an Err if that is not true.
Do you mean that since we ignored the option in the case of a Save dialog, we should not return an error, but the whole filenames vec (essentially avoid this check)?
There was a problem hiding this comment.
What I meant was: in the case of a Save dialog, we should return an error if there's more than one filename, even if options.multi_selection is true. (This is what the unwrap at line 945 assumes)
There was a problem hiding this comment.
Ah, ok, so instead of checking for && !options.multi_selection we should do:
filenames if filenames.len() > 1 && action == FileChooserAction::Save => {to uphold what we write later in the other comment, right?
There was a problem hiding this comment.
Yes, but you will need to check both the action and options.multi_selection because we still need to handle the case of open dialogs that expect a single file.
There was a problem hiding this comment.
Ok, fixing it now
| @@ -927,7 +940,9 @@ impl WindowState { | |||
| ) | |||
| .ok() | |||
| .map(|s| FileInfo { | |||
| path: s.into(), | |||
| // `get_file_dialog_path` should make sure one and only one | |||
| // file is passed | |||
There was a problem hiding this comment.
Let's be confident here :) "get_file_dialog_path guarantees that save dialogs only return one path", or something like that.
There was a problem hiding this comment.
Ok, I'm going to change the comment
|
Thanks! |
Thank you for the review :) |
As noted in #1067 the multiple selection is not actually implemented in the backends.
This PR implements the multiple selection dialog for the GTK backend.
I think I might have broken other backends, butI'm opening a PR so we can discuss if this is the right solution and if needed implement it for other platforms.The CI passed, so I suppose I didn't breake other backends