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

Retain cursor if an unsupported cursor is used #1721

Closed
wants to merge 1 commit into from

Conversation

eXpl0it3r
Copy link
Member

Description

While testing #1688, I ran into random crashes and after a while I realized that the switch-case construct didn't cover all cursor types - as documented - which then lead to random crashes when picking the unsupported cursors.

Tasks

  • Tested on macOS

How to test this PR?

You can use this example that counts through all the cursor types and will crash with master right now.

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

int main()
{
    auto window = sf::RenderWindow{{800, 600, 32}, "SFML Window"};
    window.setFramerateLimit(60);

    auto cursor = sf::Cursor{};

    auto start = sf::Cursor::Arrow;
    auto current = sf::Cursor::Arrow;
    auto end = sf::Cursor::NotAllowed;

    cursor.loadFromSystem(current);
    window.setMouseCursor(cursor);
    std::cout << "New cursor type: " << current << "\n";

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

            if ((event.type == sf::Event::KeyPressed))
            {
                switch (event.key.code)
                {
                    case sf::Keyboard::Escape:
                    {
                        window.close();
                        return 0;
                    }
                    case sf::Keyboard::Up:
                        if(current < end)
                        {
                            current = static_cast<sf::Cursor::Type>(current + 1);
                            cursor.loadFromSystem(current);
                            window.setMouseCursor(cursor);
                            std::cout << "New cursor type: " << current << "\n";
                        }
                        break;
                    case sf::Keyboard::Down:
                        if(current > start)
                        {
                            current = static_cast<sf::Cursor::Type>(current - 1);
                            cursor.loadFromSystem(current);
                            window.setMouseCursor(cursor);
                            std::cout << "New cursor type: " << current << "\n";
                        }
                        break;
                }
            }
        }


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

Not all system cursor types are supported on macOS, as such the switch
case provides a default, which however didn't retain the just previously
release cursor, which then lead to random crashes
@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Nov 29, 2020
@eXpl0it3r eXpl0it3r moved this from Discussion to Review & Testing in SFML 2.6.0 Nov 29, 2020
@eXpl0it3r eXpl0it3r added this to the 2.6 milestone Nov 29, 2020
default: return false;
default:
if (m_cursor)
[m_cursor retain];
Copy link
Member Author

Choose a reason for hiding this comment

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

If I understand the memory model of Obj-C correctly, this is not guaranteed to work, as the release at the top of the function, will allow the releasing of the cursor immediately, so one wouldn't be able to retain it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think the correct approach here is to set the member to nil after releasing it, as if it's deallocated we end up with a dangly pointer, have got a PR incoming

@eXpl0it3r
Copy link
Member Author

Superseded by #1736

@eXpl0it3r eXpl0it3r closed this Jan 6, 2021
SFML 2.6.0 automation moved this from Review & Testing to Done Jan 6, 2021
@eXpl0it3r eXpl0it3r deleted the bugfix/macos-cursor-retain branch January 6, 2021 23:46
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.

None yet

3 participants