Fix missing return value in closeButtonPressed.#21792
Merged
Merged
Conversation
Accidentally removed in f7a432a.
anntzer
referenced
this pull request
Nov 29, 2021
Also, explicitly checking for whether zoombutton/panbutton are non-null is unnecessary, as objc explicitly defines sending a message to null as a noop. Also, they should never be nil anyways...
greglucas
approved these changes
Nov 29, 2021
Contributor
greglucas
left a comment
greglucas
left a comment
There was a problem hiding this comment.
This update looks good.
For the other updates:
-
-Werrorseems to compile fine for me locally without any warnings, so I agree this seems sensible to add in. -
The manager isn't available on the view there, so I think we'd have to store that somewhere on the window and then call super to get to that method. However, I'm a bit confused why we need that chunk of code at all... I think that even calling
window.close()programatically would have closed the window by this point, so we shouldn't have to worry about that case?
Dropping that code works for me interactively with multiple windows as expected.
diff --git a/src/_macosx.m b/src/_macosx.m
index 64875fe7f1..5e3bd4fc96 100755
--- a/src/_macosx.m
+++ b/src/_macosx.m
@@ -226,7 +226,6 @@ - (void)windowDidResize:(NSNotification*)notification;
- (View*)initWithFrame:(NSRect)rect;
- (void)setCanvas: (PyObject*)newCanvas;
- (void)windowWillClose:(NSNotification*)notification;
-- (BOOL)windowShouldClose:(NSNotification*)notification;
- (BOOL)isFlipped;
- (void)mouseEntered:(NSEvent*)event;
- (void)mouseExited:(NSEvent*)event;
@@ -1291,7 +1290,11 @@ - (NSRect)constrainFrameRect:(NSRect)rect toScreen:(NSScreen*)screen
return suggested;
}
-- (BOOL)closeButtonPressed { gil_call_method(manager, "close"); }
+- (BOOL)closeButtonPressed
+{
+ gil_call_method(manager, "close");
+ return YES;
+}
- (void)close
{
@@ -1506,27 +1509,6 @@ - (void)windowWillClose:(NSNotification*)notification
PyGILState_Release(gstate);
}
-- (BOOL)windowShouldClose:(NSNotification*)notification
-{
- NSWindow* window = [self window];
- NSEvent* event = [NSEvent otherEventWithType: NSEventTypeApplicationDefined
- location: NSZeroPoint
- modifierFlags: 0
- timestamp: 0.0
- windowNumber: 0
- context: nil
- subtype: WINDOW_CLOSING
- data1: 0
- data2: 0];
- [NSApp postEvent: event atStart: true];
- if ([window respondsToSelector: @selector(closeButtonPressed)]) {
- BOOL closed = [((Window*) window) closeButtonPressed];
- /* If closed, the window has already been closed via the manager. */
- if (closed) { return NO; }
- }
- return YES;
-}
-
- (void)mouseEntered:(NSEvent *)event
{
PyGILState_STATE gstate;
Contributor
|
I'm pretty sure when I made that PR matplotlib wouldn't compile with -Werror, and it didn't seem trivial to make that a possibility. |
QuLogic
approved these changes
Nov 29, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Accidentally removed in f7a432a.
Closes f7a432a#r61013322. Would be nice if we could run with -Werror instead, which would have caught this, but I assume @dopplershift had a good reason to only use -Werror=unguarded-availability in #17956?
It's also not completely clear to me why the only call to closeButtonPressed (in windowShouldClose) is protected by respondsToSelector, and whether it could just be inlined there instead, using something like
Still, this patch should fix the main problem at hand...
PR Summary
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).