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

Pass the on_result argument to the parent FileDialog constructors #1947

Merged
merged 4 commits into from
May 24, 2023

Conversation

browniebroke
Copy link
Contributor

@browniebroke browniebroke commented May 22, 2023

I was trying to use the on_result argument for the self.main_window.select_folder_dialog(...) method and I my callback wasn't called. I looked at the code and noticed that some arguments added in #1464 were not being passed to the parent constructor.

I'm not sure if there is a particular reason for it, or if it was just missed. If the callback shouldn't be used, maybe it should be dropped from the docs.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Signed-off-by: Bruno Alla <alla.brunoo@gmail.com>
@freakboy3742
Copy link
Member

Thanks for the report and fix. FWIW, I think this is oversight/copy&paste error; the on_result handler is passed through on Windows and GTK. It wasn't picked up in previous testing because all the example code uses the await form of result handling.

You've marked the PR as draft; is there something you're planning to add or change in this PR? The only thing I can think you could add would be a testbed test, but that's definitely on the hard end. If you're up for a challenge, we'd love to have a testbed test of dialogs, but I can't point you at an existing example.

@browniebroke browniebroke marked this pull request as ready for review May 23, 2023 07:22
@browniebroke
Copy link
Contributor Author

Thanks for the prompt response. The PR was marked as draft as I wanted to check that CI passed and that I covered everything, including tests. I tried to look at the existing testbed tests and it does indeed sound a bit too tricky to incorporate dialog, so I'll leave it as is.

I didn't realise that I could get to the result using await, that seems like a nicer way of doing it actually.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

I've made a small tweak to the release note, but otherwise this is good to go. Thanks for the PR!

@freakboy3742 freakboy3742 merged commit ef85294 into beeware:main May 24, 2023
@browniebroke browniebroke deleted the fix/on-result-handler branch May 24, 2023 07:07
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.

2 participants