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

Do only handle events that correspond to the VM window #295

Merged
merged 1 commit into from
Oct 23, 2018
Merged

Do only handle events that correspond to the VM window #295

merged 1 commit into from
Oct 23, 2018

Conversation

guillep
Copy link
Contributor

@guillep guillep commented Oct 22, 2018

While playing with SDL on OSX we have noticed that the main window in the VM is randomly eating events that belong to other windows. This happens because we are consuming events regardless the window they come from.

The corresponding code is in the method pumpRunLoopEventSendAndSignal: in
https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/Cog/platforms/iOS/vm/OSX/sqSqueakOSXApplication%2Bevents.m

I've playing around with making the VM consume only events that come from its own window or from no window (window=null). This seems to work correctly, and I've had run into no issues in the last couple of hours.

I've put a guard so my code is only valid for PHAROVM builds, but I'd gladly remove the guards if people think this is a good move for all the community.

@eliotmiranda
Copy link
Contributor

Please do remove the guard. This is clearly the right thing to do on any flavour. Thanks for this important fix!!

@clementbera
Copy link
Contributor

clementbera commented Oct 22, 2018

Do you want to review John since I believe you have good OSX experience?

If no one else wants to review I'll merge wednesday afternoon, unless someone has already merged before.

Thanks Guille for this fix. Was very annoying.

@johnmci
Copy link
Contributor

johnmci commented Oct 22, 2018 via email

@johnmci
Copy link
Contributor

johnmci commented Oct 22, 2018

Ok it looks ok, not sure if it would affect the older multi-window support but I’m not sure anyone used that anyway.

@clementbera
Copy link
Contributor

Thanks John. I was scared because last time someone tried to fix this it had side effect on some events in the main window.

I merge then. We will see if someone complains.

@guillep
Copy link
Contributor Author

guillep commented Oct 23, 2018

I've posted #296 removing the PharoVM guard and adding some comments.

@guillep
Copy link
Contributor Author

guillep commented Oct 23, 2018

BTW Thanks!

fniephaus added a commit that referenced this pull request Mar 2, 2020
@guillep introduced some additional logic to filter out events that do not correspond to the VM window. The about panel and the fullscreen frame are also alien windows, but were not receiving any events because of this change. As a consequence, these events will forever remain in the queue and therefore, the buttons of the fullscreen frame and about panel cannot be clicked.
This commit reverts all changes back to the previous version that consumes all events and forwards them to NSApp.
In order to avoid breaking PharoVM, we do all of this behind an `#ifdef PharoVM`.

Relevant changes:
#295
#300
tesonep pushed a commit to tesonep/opensmalltalk-vm that referenced this pull request Jul 13, 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.

None yet

4 participants