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 destroy to Drawing to close window programatically #245

Merged
merged 4 commits into from
Jul 16, 2021

Conversation

sathwikmatsa
Copy link
Contributor

Resolves #181
Modified examples/runtest.rs to use Drawing::destroy() and tested it on Mac.

Copy link
Owner

@sunjay sunjay left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to implement this feature! Unfortunately, this repo doesn't currently support rustfmt (a lot of the formatting decisions made by that tool aren't super desirable). Could you update your PR to only contain changes relevant to the feature you're implementing? You can usually do this by using a tool like git gui or sourcetree to only select the changes you want to submit.

As far as I can see you've done a great job implementing this. No major feedback other than a few small comments. I'd be happy to take another look and merge once we've narrowed this down to only the changes needed for #181. Thanks again! 🎉 😁

src/drawing.rs Show resolved Hide resolved
Self { event_loop }
}

pub fn exit(&self) -> Result<(), EventLoopClosed> {
Copy link
Owner

Choose a reason for hiding this comment

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

nit: could you move this method down to the bottom so it matches the order in the enum? (makes it a bit easier to see that each variant of the enum has a corresponding method here)

@@ -14,6 +14,10 @@ impl EventLoopNotifier {
Self {}
}

pub fn exit(&self) -> Result<(), EventLoopClosed> {
Copy link
Owner

Choose a reason for hiding this comment

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

Please also move this method down per my previous comment.

Copy link
Owner

@sunjay sunjay left a comment

Choose a reason for hiding this comment

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

Great work @sathwikmatsa! Appreciate you writing docs and an example for the method and knowing to add the compile_error! call to the runtest example.

@sunjay sunjay merged commit c9918c4 into sunjay:master Jul 16, 2021
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.

Add a way to close the window
2 participants