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

Fixed mouse clicks not activating windows (Win32) #457

Merged
merged 1 commit into from Sep 25, 2013

Conversation

Projects
None yet
8 participants
@MarioLiebisch
Member

MarioLiebisch commented Aug 28, 2013

  • This fixes issue #437, except Alt+F4, which would be a different issue?
  • This is essentially based around @All8Up's suggestion.
  • The new member variable m_focused is meant to be exposed later on so the user is able to determine whether the window is active without doing separate event tracking.
  • Activating mouse clicks aren't passed to the event queue. This is something not really consistent across games or applications in general, but I prefer this behavior (avoids triggering anything by accident).
@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 28, 2013

Member

This fixes issue #437, except Alt+F4, which would be a different issue?

I don't think it is a different one. So before I can validate your patch, you must first check the Alt+F4 issue: you can test the revision before the "fix" I made (the one that broke everything), and see whether the Alt+F4 problem was also caused by this commit or not.

(I'm really sorry, I can't do this myself right now)

Member

LaurentGomila commented Aug 28, 2013

This fixes issue #437, except Alt+F4, which would be a different issue?

I don't think it is a different one. So before I can validate your patch, you must first check the Alt+F4 issue: you can test the revision before the "fix" I made (the one that broke everything), and see whether the Alt+F4 problem was also caused by this commit or not.

(I'm really sorry, I can't do this myself right now)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Aug 28, 2013

Member

No worries, will do.

Member

MarioLiebisch commented Aug 28, 2013

No worries, will do.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Aug 28, 2013

Member

Ah... dug a bit deeper and this is indeed related (SetCapture() disabling the system shortcuts like Alt+F4). Will have to come up with something different.

Member

MarioLiebisch commented Aug 28, 2013

Ah... dug a bit deeper and this is indeed related (SetCapture() disabling the system shortcuts like Alt+F4). Will have to come up with something different.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 28, 2013

Member

Ok, thanks.

So it's pretty clear that SetCapture is not the right solution, and we should start by reverting to the previous implementation.

Member

LaurentGomila commented Aug 28, 2013

Ok, thanks.

So it's pretty clear that SetCapture is not the right solution, and we should start by reverting to the previous implementation.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Aug 28, 2013

Member

No, it actually works, but the current approach is wrong. Working on a solution, it's rather trivial once you've got the basics working.

Member

MarioLiebisch commented Aug 28, 2013

No, it actually works, but the current approach is wrong. Working on a solution, it's rather trivial once you've got the basics working.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Aug 28, 2013

Member

Okay, new revision should be a lot cleaner and solves the issue as a whole rather than applying a workaround.

Member

MarioLiebisch commented Aug 28, 2013

Okay, new revision should be a lot cleaner and solves the issue as a whole rather than applying a workaround.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 28, 2013

Member

If you did something different from the old implementation, would you mind explaning?

Member

LaurentGomila commented Aug 28, 2013

If you did something different from the old implementation, would you mind explaning?

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Aug 28, 2013

Member

The difference to my first fix or the current implementation?

My previous fix tried to manually switch focus when you click inside the window, but this didn't stop SetCapture() blocking shortcut keys.

This new fix splits event generation (mouse left/entered) as well as the whole code around SetCapture(). SetCapture() is called once you press a mouse button and the capture ends once you release all buttons, no matter where the cursor is.

The initial problem in short:

  • When you call SetCapture() your window will receive all mouse events as long as at least one mouse button is pressed.
  • SetCapture() has been set whenever the mouse was inside the window, no matter the actual status of the buttons.
  • ReleaseCapture() was only called (during a mouse movement) when the mouse left the window.
  • The problem: The window never received any mouse move event once the cursor left the Window without having a button pressed, so the capture was never released.
  • As long as the capture is present, you can't activate a window by clicking on it and you can't use shortcuts such als Alt+F4.
Member

MarioLiebisch commented Aug 28, 2013

The difference to my first fix or the current implementation?

My previous fix tried to manually switch focus when you click inside the window, but this didn't stop SetCapture() blocking shortcut keys.

This new fix splits event generation (mouse left/entered) as well as the whole code around SetCapture(). SetCapture() is called once you press a mouse button and the capture ends once you release all buttons, no matter where the cursor is.

The initial problem in short:

  • When you call SetCapture() your window will receive all mouse events as long as at least one mouse button is pressed.
  • SetCapture() has been set whenever the mouse was inside the window, no matter the actual status of the buttons.
  • ReleaseCapture() was only called (during a mouse movement) when the mouse left the window.
  • The problem: The window never received any mouse move event once the cursor left the Window without having a button pressed, so the capture was never released.
  • As long as the capture is present, you can't activate a window by clicking on it and you can't use shortcuts such als Alt+F4.
@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 28, 2013

Member

The difference to my first fix or the current implementation?

I meant, the difference with my old implementation which almost worked (the one before I broke everything) ;)

Member

LaurentGomila commented Aug 28, 2013

The difference to my first fix or the current implementation?

I meant, the difference with my old implementation which almost worked (the one before I broke everything) ;)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Aug 28, 2013

Member

See the second part of my last comment starting with "The initial problem...". I now release the capture once you no longer press the mouse button, no matter where the cursor is and only reacquire it when needed (i.e. when a button is pressed).

Edit: Ah, you mean the non-broken one. Have to check, but I'm sure in the end it's due to missing mouse events outside the window when not using SetCapture().

Edit 2: Yes, that seems to be the difference. In the end you'll need both, the old as well as the new (standalone broken) handling to catch all cases and not lose input events when moving the cursor outside.

Member

MarioLiebisch commented Aug 28, 2013

See the second part of my last comment starting with "The initial problem...". I now release the capture once you no longer press the mouse button, no matter where the cursor is and only reacquire it when needed (i.e. when a button is pressed).

Edit: Ah, you mean the non-broken one. Have to check, but I'm sure in the end it's due to missing mouse events outside the window when not using SetCapture().

Edit 2: Yes, that seems to be the difference. In the end you'll need both, the old as well as the new (standalone broken) handling to catch all cases and not lose input events when moving the cursor outside.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 28, 2013

Member

What about activating clicks now? Are they still lost? Were they reported in the working implementation?

Member

LaurentGomila commented Aug 28, 2013

What about activating clicks now? Are they still lost? Were they reported in the working implementation?

@All8Up

This comment has been minimized.

Show comment
Hide comment
@All8Up

All8Up Aug 28, 2013

I've been testing the latest changes Mario put up locally while working on my UI and simple tools stuff and it works like a charm. The initial mouse click does come through, which is as I expected and preferred but since it is ordered correctly it is easy to eat in the cases where I do not want it to take immediate action. Alt F4, loss of focus when clicking dragging of the window and all that seem to be nice and solid etc. I can't guarantee I've not missed other edge cases or related bugs while putting this to use, but it seems to be working.

All8Up commented Aug 28, 2013

I've been testing the latest changes Mario put up locally while working on my UI and simple tools stuff and it works like a charm. The initial mouse click does come through, which is as I expected and preferred but since it is ordered correctly it is easy to eat in the cases where I do not want it to take immediate action. Alt F4, loss of focus when clicking dragging of the window and all that seem to be nice and solid etc. I can't guarantee I've not missed other edge cases or related bugs while putting this to use, but it seems to be working.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Aug 28, 2013

Member

Nothing is lost, clicking inside the window to activate it will also trigger a MouseButtonPressed event. They were never lost, except in the first proposed fix.

Member

MarioLiebisch commented Aug 28, 2013

Nothing is lost, clicking inside the window to activate it will also trigger a MouseButtonPressed event. They were never lost, except in the first proposed fix.

@FRex

This comment has been minimized.

Show comment
Hide comment
@FRex

FRex Aug 28, 2013

Contributor

So the click that gained focus comes before or after the actual focus event in Mario's fix?

Contributor

FRex commented Aug 28, 2013

So the click that gained focus comes before or after the actual focus event in Mario's fix?

@All8Up

This comment has been minimized.

Show comment
Hide comment
@All8Up

All8Up Aug 28, 2013

The tests all show focus gain shows up first as appropriate, then the mouse click as expected. This is the preferred and proper solution I would think, so cheers..

All8Up commented Aug 28, 2013

The tests all show focus gain shows up first as appropriate, then the mouse click as expected. This is the preferred and proper solution I would think, so cheers..

@FRex

This comment has been minimized.

Show comment
Hide comment
@FRex

FRex Aug 28, 2013

Contributor

Ok, I misread.

Contributor

FRex commented Aug 28, 2013

Ok, I misread.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Aug 28, 2013

Member

It should generate events in the proper order every time. All8Up's initial suggestion caused wrong order of events due to the way it focused the window after processing the click. As this is no longer the case you should see no difference to the expected behavior.

Member

MarioLiebisch commented Aug 28, 2013

It should generate events in the proper order every time. All8Up's initial suggestion caused wrong order of events due to the way it focused the window after processing the click. As this is no longer the case you should see no difference to the expected behavior.

@All8Up

This comment has been minimized.

Show comment
Hide comment
@All8Up

All8Up Aug 28, 2013

Of course the downside is that testing on OsX shows that you don't get the initial mouse click which caused the activation. This is going to be a problem for my little project, grr.. Either the initial click should be added to OsX or it should be eaten on Windows for consistency sake. If it gets eaten then I'm going to have to add set focus abilities to SFML on OsX and Windows (don't do much Linux unfortunately) in order to fix up the annoying double click on tools buttons issue in my project.

All8Up commented Aug 28, 2013

Of course the downside is that testing on OsX shows that you don't get the initial mouse click which caused the activation. This is going to be a problem for my little project, grr.. Either the initial click should be added to OsX or it should be eaten on Windows for consistency sake. If it gets eaten then I'm going to have to add set focus abilities to SFML on OsX and Windows (don't do much Linux unfortunately) in order to fix up the annoying double click on tools buttons issue in my project.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Aug 28, 2013

Member

Guess it's easier to add rather than dropping it. After all you should probably remove the mouse up as well, which could get rather messy if you're clicking with multiple buttons at once or quick succession.

Member

MarioLiebisch commented Aug 28, 2013

Guess it's easier to add rather than dropping it. After all you should probably remove the mouse up as well, which could get rather messy if you're clicking with multiple buttons at once or quick succession.

@All8Up

This comment has been minimized.

Show comment
Hide comment
@All8Up

All8Up Aug 28, 2013

Yup, unfortunately looking through the cocoa code shows that it is being eaten at the Os level it seems which makes things difficult to say the least. Have to simulate both down and up even though OsX is going to be ignoring the release event whenever it happens.

All8Up commented Aug 28, 2013

Yup, unfortunately looking through the cocoa code shows that it is being eaten at the Os level it seems which makes things difficult to say the least. Have to simulate both down and up even though OsX is going to be ignoring the release event whenever it happens.

@All8Up

This comment has been minimized.

Show comment
Hide comment
@All8Up

All8Up Aug 29, 2013

Not sure I'll suggest this till I do some more checking, but it makes the app work correctly on both platforms and I can't seem to find any side effects. Linux will have to be checked by others, I don't work there much. I suppose I really should go ahead and make a branch but this is a relatively small item and the only reason I've been doing the hacking at the moment is to get my app working again after moving from 2.0 to 2.1.

// In WindowImplCocoa.mm
// Add to the end of WindowImplCocoa::windowGainedFocus(void)
// Simulate a mouse down/up in order to be consistent with Windozer.
// Only have to worry about left mouse button since other buttons will not
// cause the window to focus but do get transmitted to the window.
NSPoint pos = [NSEvent mouseLocation];
pos.y = sf::VideoMode::getDesktopMode().height - pos.y;

CGEventRef downEvent = CGEventCreateMouseEvent( NULL, kCGEventLeftMouseDown, pos, 0 );
CGEventRef upEvent = CGEventCreateMouseEvent( NULL, kCGEventLeftMouseUp, pos, 0 );

CGEventPost( kCGHIDEventTap, downEvent );
CGEventPost( kCGHIDEventTap, upEvent );

CFRelease( downEvent );
CFRelease( upEvent );

All8Up commented Aug 29, 2013

Not sure I'll suggest this till I do some more checking, but it makes the app work correctly on both platforms and I can't seem to find any side effects. Linux will have to be checked by others, I don't work there much. I suppose I really should go ahead and make a branch but this is a relatively small item and the only reason I've been doing the hacking at the moment is to get my app working again after moving from 2.0 to 2.1.

// In WindowImplCocoa.mm
// Add to the end of WindowImplCocoa::windowGainedFocus(void)
// Simulate a mouse down/up in order to be consistent with Windozer.
// Only have to worry about left mouse button since other buttons will not
// cause the window to focus but do get transmitted to the window.
NSPoint pos = [NSEvent mouseLocation];
pos.y = sf::VideoMode::getDesktopMode().height - pos.y;

CGEventRef downEvent = CGEventCreateMouseEvent( NULL, kCGEventLeftMouseDown, pos, 0 );
CGEventRef upEvent = CGEventCreateMouseEvent( NULL, kCGEventLeftMouseUp, pos, 0 );

CGEventPost( kCGHIDEventTap, downEvent );
CGEventPost( kCGHIDEventTap, upEvent );

CFRelease( downEvent );
CFRelease( upEvent );

@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Aug 29, 2013

What if someone focuses the window without clicking on the window with the mouse though?

retep998 commented Aug 29, 2013

What if someone focuses the window without clicking on the window with the mouse though?

@All8Up

This comment has been minimized.

Show comment
Hide comment
@All8Up

All8Up Aug 29, 2013

Oops, forgot to include the check for lbutton down.. It's in an if statement.. I'll go get that code and post it here.

Surround the call in "if( HIDInputManager::getInstance().isMouseButtonPressed( sf::Mouse::Button::Left )".

It still leaves the edge case of if you have the mouse button down during alt/tab or clicking on the dock, but the dock should be easy to filter via mouse over test and the alt-tab seems like a pretty minor glitch. The real problem is that I don't believe there is any source information which comes with the focus calls in Cocoa, so this is all pretty much a hack still. I will look into the gained focus event and see if there is any other information possible to extract that could clean this up. Unfortunately, I just didn't want to have "#if OSX #else" trash all over my code to deal with this and figured as a quick solution this was not too bad.

All8Up commented Aug 29, 2013

Oops, forgot to include the check for lbutton down.. It's in an if statement.. I'll go get that code and post it here.

Surround the call in "if( HIDInputManager::getInstance().isMouseButtonPressed( sf::Mouse::Button::Left )".

It still leaves the edge case of if you have the mouse button down during alt/tab or clicking on the dock, but the dock should be easy to filter via mouse over test and the alt-tab seems like a pretty minor glitch. The real problem is that I don't believe there is any source information which comes with the focus calls in Cocoa, so this is all pretty much a hack still. I will look into the gained focus event and see if there is any other information possible to extract that could clean this up. Unfortunately, I just didn't want to have "#if OSX #else" trash all over my code to deal with this and figured as a quick solution this was not too bad.

@Wizzard033

This comment has been minimized.

Show comment
Hide comment
@Wizzard033

Wizzard033 Sep 22, 2013

I've been using this pull request for a week and it seems to have fixed all the
inconsistencies that the Windows version of SFML had with Linux when it comes to events.

Edit: Including the old implementation that was thought to be working fine - it fixed inconsistencies
Fixes #455

Wizzard033 commented Sep 22, 2013

I've been using this pull request for a week and it seems to have fixed all the
inconsistencies that the Windows version of SFML had with Linux when it comes to events.

Edit: Including the old implementation that was thought to be working fine - it fixed inconsistencies
Fixes #455

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 22, 2013

Member

Nice, thanks for the feedback :)

Member

LaurentGomila commented Sep 22, 2013

Nice, thanks for the feedback :)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 24, 2013

Member

Is this getting accepted anytime soon? :)

Member

eXpl0it3r commented Sep 24, 2013

Is this getting accepted anytime soon? :)

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 24, 2013

Member

It must be formatted properly before it can be accepted.

There's an if (...) {, missing curly braces in switch case, missing line breaks, and a lot of minor stuff that won't change anything except that I'll be happy and merge the code once it's corrected ;)

And untrackMouse() should be called in cleanup(). Since the code in trackMouse and untrackMouse is almost the same, a trackMouse(bool) function would be even better.

Member

LaurentGomila commented Sep 24, 2013

It must be formatted properly before it can be accepted.

There's an if (...) {, missing curly braces in switch case, missing line breaks, and a lot of minor stuff that won't change anything except that I'll be happy and merge the code once it's corrected ;)

And untrackMouse() should be called in cleanup(). Since the code in trackMouse and untrackMouse is almost the same, a trackMouse(bool) function would be even better.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 24, 2013

Member

Always those tiny things! :) Cleaned it up a bit, hope didn't miss anything. As for naming the tracking function, I've used setTracking() for now (as it's essentially for touch input as well), but I'll leave the final naming up to you.

Member

MarioLiebisch commented Sep 24, 2013

Always those tiny things! :) Cleaned it up a bit, hope didn't miss anything. As for naming the tracking function, I've used setTracking() for now (as it's essentially for touch input as well), but I'll leave the final naming up to you.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 24, 2013

Member

Almost :)

All the switch cases are enclosed in { } (in the big event switch), it would be cool if you could do it too.

Also, for ifs followed by a single line, I usually omit the pair of { }

And don't remove the extra space between case labels and ':'. This is totally irrelevant here, you just break the current style.

Member

LaurentGomila commented Sep 24, 2013

Almost :)

All the switch cases are enclosed in { } (in the big event switch), it would be cool if you could do it too.

Also, for ifs followed by a single line, I usually omit the pair of { }

And don't remove the extra space between case labels and ':'. This is totally irrelevant here, you just break the current style.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 24, 2013

Member

Okay, will have a look. As for the extra spaces in case labels: Those were inconsistent to begin with, so I removed those (as others were lacking them already). Would be something to unify everywhere I guess? Edit: Hm not the case here, think it's been something else I confused with these.

Member

MarioLiebisch commented Sep 24, 2013

Okay, will have a look. As for the extra spaces in case labels: Those were inconsistent to begin with, so I removed those (as others were lacking them already). Would be something to unify everywhere I guess? Edit: Hm not the case here, think it's been something else I confused with these.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 24, 2013

Member

There you go. It was indeed inconsistent (resize events).

Member

MarioLiebisch commented Sep 24, 2013

There you go. It was indeed inconsistent (resize events).

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 24, 2013

Member

The final one: I never put a comment right below a line of code, I always leave an empty line.

And as a bonus, it would be cleaner if you could merge your commits and provide a single clean one :)

Sorry about the inconsistencies. My current style (at work and in new projects) is different from SFML style, which was decided in 2007. So sometimes I may mix them :)

Member

LaurentGomila commented Sep 24, 2013

The final one: I never put a comment right below a line of code, I always leave an empty line.

And as a bonus, it would be cleaner if you could merge your commits and provide a single clean one :)

Sorry about the inconsistencies. My current style (at work and in new projects) is different from SFML style, which was decided in 2007. So sometimes I may mix them :)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 24, 2013

Member

Okay, seems like this worked. And no worries, I love screwing up my own coding style between hobby, work, etc. as well. :)

Member

MarioLiebisch commented Sep 24, 2013

Okay, seems like this worked. And no worries, I love screwing up my own coding style between hobby, work, etc. as well. :)

Fixed mouse clicks not activating windows (Win32)
- This fixes issue #437.
- This also restores system shortcuts like Alt+F4 or Alt+Space.
@JuDelCo

This comment has been minimized.

Show comment
Hide comment
@JuDelCo

JuDelCo Sep 24, 2013

Finally it will be merged to the main repo :D I'm going to update my sfml copy when it happens ^^

JuDelCo commented Sep 24, 2013

Finally it will be merged to the main repo :D I'm going to update my sfml copy when it happens ^^

LaurentGomila added a commit that referenced this pull request Sep 25, 2013

Merge pull request #457 from MarioLiebisch/issue-437
Fixed mouse clicks not activating windows (Win32) (#437, #455)

@LaurentGomila LaurentGomila merged commit cd84e84 into SFML:master Sep 25, 2013

@MarioLiebisch MarioLiebisch deleted the MarioLiebisch:issue-437 branch Sep 26, 2013

@eXpl0it3r eXpl0it3r added bug labels May 13, 2014

@JordanFitz JordanFitz referenced this pull request Feb 6, 2016

Closed

Window Focus on Click #214

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment