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

Process Cocoa events after window close #1814

Merged
merged 2 commits into from Aug 12, 2021
Merged

Conversation

jqdg
Copy link
Contributor

@jqdg jqdg commented Aug 8, 2021

In recent versions of Mac OS, windows remain visible on screen ("stuck") even after closing unless events are processed. See glfw/glfw#1412 and this commit from the GLFW project. SFML also suffers from this issue.

This PR works around this issue by processing Cocoa events immediately after the window is closed.

Also, it adds a fix that should resolve #1581.

Example code to reproduce the issue (Mac OS only)

Without this PR, the window will remain visible even after it is closed (while waiting for using input from cin). With this PR, the window will be closed immediately (correct behavior).

#include <SFML/Graphics.hpp>
#include <iostream>

int main()
{
    sf::RenderWindow window(sf::VideoMode(1280, 720), "Minimal, complete and verifiable example");
    window.setFramerateLimit(60);

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                window.close();
        }

        window.clear();
        window.display();
    }

    int x;
    std::cin >> x;
    
    return 0;
}

Workaround issue where the window remains open if events are not
processed.
@eXpl0it3r
Copy link
Member

eXpl0it3r commented Aug 8, 2021

Is this related or the same issue as #1581 describes?

@jqdg
Copy link
Contributor Author

jqdg commented Aug 8, 2021

Is this related or the same issue as #1581 describes?

It's not really related, but I think that issue can be resolved with the following patch (I can add it to this PR if you want):

diff --git a/src/SFML/Window/OSX/SFWindow.h b/src/SFML/Window/OSX/SFWindow.h
index a59101a9..b3da579d 100644
--- a/src/SFML/Window/OSX/SFWindow.h
+++ b/src/SFML/Window/OSX/SFWindow.h
@@ -57,6 +57,16 @@
 ////////////////////////////////////////////////////////////
 -(BOOL)canBecomeKeyWindow;
 
+////////////////////////////////////////////////////////////
+/// \brief Allow to grab fullscreen events
+///
+/// See acceptsFirstResponder documentation above.
+///
+/// \return YES
+///
+////////////////////////////////////////////////////////////
+-(BOOL)canBecomeMainWindow;
+
 ////////////////////////////////////////////////////////////
 /// \brief Prevent system alert
 ///
diff --git a/src/SFML/Window/OSX/SFWindow.m b/src/SFML/Window/OSX/SFWindow.m
index ff60c6c5..507f258b 100644
--- a/src/SFML/Window/OSX/SFWindow.m
+++ b/src/SFML/Window/OSX/SFWindow.m
@@ -45,6 +45,13 @@
 }
 
 
+////////////////////////////////////////////////////////
+-(BOOL)canBecomeMainWindow
+{
+    return YES;
+}
+
+
 ////////////////////////////////////////////////////////
 -(void)keyDown:(NSEvent*)theEvent
 {
diff --git a/src/SFML/Window/OSX/WindowImplCocoa.mm b/src/SFML/Window/OSX/WindowImplCocoa.mm
index 091b7b2d..b4453a98 100644
--- a/src/SFML/Window/OSX/WindowImplCocoa.mm
+++ b/src/SFML/Window/OSX/WindowImplCocoa.mm
@@ -154,7 +154,11 @@ WindowImplCocoa::~WindowImplCocoa()
     // Put the next window in front, if any.
     NSArray* windows = [NSApp orderedWindows];
     if ([windows count] > 0)
-        [[windows objectAtIndex:0] makeKeyAndOrderFront:nil];
+    {
+        NSWindow *nextWindow = [windows objectAtIndex:0];
+        if ([nextWindow isVisible])
+            [nextWindow makeKeyAndOrderFront:nil];
+    }
 
     drainThreadPool(); // Make sure everything was freed
     // This solve some issue when sf::Window::Create is called for the

@eXpl0it3r
Copy link
Member

Either added to this one or as separate PR, both would work for me.

Highly appreciated! 🙂

@jqdg
Copy link
Contributor Author

jqdg commented Aug 9, 2021

Done. I added the fix to this PR.

@eXpl0it3r
Copy link
Member

Successfully tested the originally mentioned issue.

Even though I can't reproduce the issue mentioned in #1581 the logic seems, to my little understanding of this, to be okay and does work.

@eXpl0it3r eXpl0it3r added this to the 2.6 milestone Aug 9, 2021
@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Aug 9, 2021
@eXpl0it3r eXpl0it3r moved this from Discussion to Ready in SFML 2.6.0 Aug 9, 2021
@jqdg
Copy link
Contributor Author

jqdg commented Aug 9, 2021

Thanks for the quick review!

I want to edit one of the Doxygen comments. Will it cause any issues if I force push to this branch?

@eXpl0it3r
Copy link
Member

Not at all, that's actually our preferred way of operation! 😉

@eXpl0it3r eXpl0it3r merged commit 86617c0 into SFML:master Aug 12, 2021
SFML 2.6.0 automation moved this from Ready to Done Aug 12, 2021
@eXpl0it3r
Copy link
Member

eXpl0it3r commented Aug 12, 2021

Thanks a lot for these fixes! 🙂

If you see any other macOS things that can be improved let us, we're currently lacking some stronger macOS experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

Fullscreen leaves zombie window open on macOS
3 participants