-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
FIX: macos: Add update capability to interval/singleshot timer properties #29018
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
Conversation
9a99838
to
e60da20
Compare
# GTK4 on macos runners produces about 3x as many calls as expected | ||
# It works locally and on Linux though, so only skip when running on CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really annoying to wrestle with CI sometimes. I have no idea why the macos CI runners would have ~60 calls, but my local version and the linux runners get the correct ~20 calls.
If anyone has better ideas for how to test these timers let me know too. Each new test/"pause" we add here adds 2 seconds * nbackends / njobs
to the test runs and it looks like this test is currently one of the longest running in the suite at around ~10s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did plt.pause
not run for the expected pause_time
, perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good thought! That led me to a few refactors here which I pushed up as separate commits.
- I removed
plt.pause()
and went directly tostart_event_loop()
now in case there was additional overhead in any of the pause draw calls. - I parallelized the tests now so we only run two "pauses" and test multiple things during each pause.
- I noticed that GTK doesn't implement its own
start_event_loop()
and our current default one inbackend_bases.py
could run longer than expected ifflush_events()
added time to the loop. So I changed this in the final commit to be dependent on the run-time, not the number of iterations of sleep.
f6e282b
to
b98b898
Compare
The implementation of start_event_loop would previously just count the number of sleeps that occurred. But this could lead to longer event loop times if flush_events() added time into the loop. We want the condition to be dependent on the end-time so we don't run our loop longer than necessary.
87bfe62
to
2d7a86f
Compare
Closing in favor of doing this all in one consolidated PR: #29062 |
PR summary
As noted by @dopplershift here #28997 (comment)
The macos timers didn't get updated when a user would update the timer interval
timer.interval = newvalue
(in that example a new timer is created and then an interval is set with the delay).This adds tests for the timer updates as well.
PR checklist