Skip to content

Add file filtering to gtk dialogs.#903

Merged
xStrom merged 3 commits intolinebender:masterfrom
jneem:gtk-dialog
May 10, 2020
Merged

Add file filtering to gtk dialogs.#903
xStrom merged 3 commits intolinebender:masterfrom
jneem:gtk-dialog

Conversation

@jneem
Copy link
Member

@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
Member 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.

@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
Member 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
Member 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