Skip to content

FIX: macosx flush_events should process all events #23636

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 20, 2022
Merged

FIX: macosx flush_events should process all events #23636

merged 1 commit into from
Aug 20, 2022

Conversation

greglucas
Copy link
Contributor

PR Summary

Rather than just bursting the runloop for a short amount of time, we can run it until all pending events have been processed. In this example on main, the buttons are all unable to be pushed and close/minimize are grayed out. The buttons should all work with this update.

import matplotlib, random
import matplotlib.pyplot as plt

matplotlib.use('macosx') #Works fine with tkagg
data = []

#Initial Draw
fig, axes = plt.subplots(1)
line, = axes.plot(data, label='Some data', color='#CC0000')
fig.canvas.draw_idle()
plt.show(block=False)

while True:
	data += [random.randint(0,50) for i in range(20)]

	tseries = range(0, len(data))
	line.set_ydata(data)
	line.set_xdata(tseries)
	axes.relim()
	axes.autoscale_view(tight=False)

	if fig.stale: fig.canvas.draw()
	fig.canvas.flush_events() #Listen for user input

closes #23327

PR Checklist

Tests and Styling

  • [N/A] Has pytest style unit tests (and pytest passes).
  • [N/A] Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@greglucas greglucas added this to the v3.6.0 milestone Aug 16, 2022
QuLogic
QuLogic approved these changes Aug 17, 2022
View reviewed changes
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

This seems consistent with what Tk does, https://github.com/tcltk/tk/blob/main/generic/tkCmds.c#L1201

@tacaswell
Copy link
Member

One way I can see this going wrong is if in the process of processing the event it adds an event to the queue. Will this cut of at "only events that were in the queue when we started" or will it get stuck?

https://developer.apple.com/documentation/foundation/nsdefaultrunloopmode should also win an award for likely correct but very unhelpful documentation.

tacaswell
tacaswell reviewed Aug 19, 2022
View reviewed changes
src/_macosx.m
NSEvent *event;
while (true) {
event = [NSApp nextEventMatchingMask: NSEventMaskAny
untilDate: [NSDate distantPast]
Copy link
Member

Choose a reason for hiding this comment

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

I do not fully understand https://developer.apple.com/documentation/appkit/nsapplication/1428485-nexteventmatchingmask . It is not clear if "expiration" here means "do not wait longer than this" or "do not return any events newer than this". If it is the second, I think grabbing the current time outside of the while loop and passing it here will prevent the perpetual-motion machine I am worried about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See recent commit. The distantFuture discussion I think indicates the second of those. https://developer.apple.com/documentation/foundation/nsdate/1415385-distantfuture

Copy link
Member

Choose a reason for hiding this comment

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

You can use the object returned by distantFuture as the date argument to wait indefinitely for the event to occur.

I read this as pointing to the first (it is a time out not a filter)?

@greglucas greglucas force-pushed the macosx-flush-events branch from 5feace7 to 4c69e36 Compare August 19, 2022 18:41
@greglucas
Copy link
Contributor Author

I pushed up that idea in the most recent commit. It seems to still work. The only thing is that we are popping the event off the queue and resending them, so I assume that "resend" event may or may-not get processed and if it doesn't wouldn't meet the criteria the second go-around. So, is the purpose here to "flush" the queue and drain it entirely, or just to try and send all events... Another option would be to create a new queue entirely and keep track ourselves, but I think some of these can be future bug fixes too.

Here is a link to some QT event processing: https://code.woboq.org/qt5/qtbase/src/plugins/platforms/cocoa/qcocoaeventdispatcher.mm.html#462

@tacaswell
Copy link
Member

I would trust the Qt code over my rambling. There is also a question of concurency here that I am not sure of. Does [NSApp sendEvent:event]; (https://developer.apple.com/documentation/appkit/nsapplication/1428359-sendevent) do that work now or schedule something on other objects that will happen the next time they get a chance to run? If they try at add events to the queue we are pulling things off of does that happen now or when we return from our function?

For the Qt code processEvents (https://doc.qt.io/qt-6/qcoreapplication.html#processEvents) should only process thing enqueue before the call so I think that means your initial commit is correct (and I do not understand this code).

@greglucas
Copy link
Contributor Author

I am out of my league here, I'm not truly a mac programmer ;)
I was basing some of this off of other event handlers within the backend as well.

matplotlib/src/_macosx.m

Lines 149 to 160 in b54b335

while (true) {
while (true) {
event = [NSApp nextEventMatchingMask: NSEventMaskAny
untilDate: [NSDate distantPast]
inMode: NSDefaultRunLoopMode
dequeue: YES];
if (!event) { break; }
[NSApp sendEvent: event];
}
CFRunLoopRun();
if (interrupted || CFReadStreamHasBytesAvailable(stream)) { break; }
}

I still think the first commit is potentially better than what we had before because the previous version was non-responsive and not really doing the right thing. This one may have the potential to get caught in the never-ending loop if an event fires 10 new events and just keeps building the event queue up, but that is likely a low-likelihood case.

Regardless, I think all of this can be considered bug-fixes and brought in after the rc even.

@greglucas greglucas force-pushed the macosx-flush-events branch from 4c69e36 to 9921e39 Compare August 19, 2022 20:00
@QuLogic
Copy link
Member

QuLogic commented Aug 19, 2022

Neither Tk nor GTK (and presumably wx along with it) special case that behaviour, so I guess we are inconsistent either way. IIRC, there was some discussion about this for the GTK4 port as well.

@QuLogic QuLogic merged commit 1456f87 into matplotlib:main Aug 20, 2022
@evanandresen evanandresen mentioned this pull request Oct 2, 2022
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell approved these changes

@QuLogic QuLogic QuLogic approved these changes

Assignees
No one assigned
Labels
GUI: MacOSX
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.

[Bug]: Event loop issue with Macosx backend
3 participants
@greglucas @tacaswell @QuLogic