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 filtering to gtk dialogs. #903

Merged
merged 3 commits into from
May 10, 2020
Merged

Add file filtering to gtk dialogs. #903

merged 3 commits into from
May 10, 2020

Conversation

jneem
Copy link
Collaborator

@jneem jneem commented May 6, 2020

Fixes GTK part of #390.

@xStrom xStrom added S-needs-review waits for review shell/gtk concerns the GTK backend labels May 6, 2020
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Generally looks good, just a few things. Also, please add a changelog entry.

Comment on lines 38 to 40
.allowed_types(vec![txt, other])
.default_type(txt);
Copy link
Member

Choose a reason for hiding this comment

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

Let's reverse the vector to [other, txt] so that this sample can show the default type working. Does it work for you? On my Ubuntu 19.10 it doesn't seem to actually choose the text file if other is first in the vector.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, it wasn't working with the other order. I've added some extra checks in dialog.rs, and now all the combinations I've tried seem to work.

druid-shell/src/platform/gtk/dialog.rs Outdated Show resolved Hide resolved
druid/examples/open_save.rs Outdated Show resolved Hide resolved
@xStrom xStrom added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels May 9, 2020
// allowed filters, but it's not ok (or at least, doesn't work in gtk)
// to provide a default filter that isn't in the (present) list
// of allowed filters.
log::warn!("default file type not found in allowed types");
Copy link
Member

Choose a reason for hiding this comment

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

This warning should be removed, because there is an explictly documented behavior for this case.

If it's None or not present in allowed_types then the first entry in allowed_types will be used as default.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But it seems unreasonable for the user to provide a default that is not even available.
If this happens, it most definitely seems to be an accident and should log a warning (on all platforms), at least I think so.

Copy link
Member

Choose a reason for hiding this comment

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

I guess that's reasonable. We can keep the warning then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that (and I can change this if it isn't desired druid behavior), if allowed_types is empty then the default type is allowed to be present: it applies a filter that cannot be changed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to be clear, my previous comment is about gtk's behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So if I interpret that code correctly, on Windows Some(default_type) is ignored if allowed_types is None. On mac the default type seems to be ignored all together.

I'm not sure what the 'correct' behavior would be (at least not the mac one).
Maybe we should allow having default be Some while allowed is None?

Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Thanks!

@xStrom xStrom merged commit 3878d31 into linebender:master May 10, 2020
@xStrom xStrom removed the S-waiting-on-author waits for changes from the submitter label May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shell/gtk concerns the GTK backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants