Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Closed
wants to merge 4 commits into from

Conversation

greglucas
Copy link
Contributor

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

Comment on lines 652 to 653
# 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
Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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 to start_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 in backend_bases.py could run longer than expected if flush_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.

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.
@greglucas
Copy link
Contributor Author

Closing in favor of doing this all in one consolidated PR: #29062

@greglucas greglucas closed this Nov 1, 2024
@greglucas greglucas deleted the macos-timer-updates branch November 1, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.