Skip to content

Add NewEvents(Init) callback to x11#1031

Merged
Osspial merged 1 commit intorust-windowing:masterfrom
rikusalminen:event_loop_test
Jul 10, 2019
Merged

Add NewEvents(Init) callback to x11#1031
Osspial merged 1 commit intorust-windowing:masterfrom
rikusalminen:event_loop_test

Conversation

@rikusalminen
Copy link
Copy Markdown
Contributor

@rikusalminen rikusalminen commented Jul 9, 2019

On X11, Event::NewEvents(StartCause::Init) was not delivered as first event.

̃I added a test case to run on an event loop without any windows attached and check that first event is NewEvents(Init), then ControlFlow::Exit and check that last event is LoopDestroyed.

I have ran this test on Linux/x11 platform only.
Help wanted: please run the new test on macos and windows.

  • Tested on all platforms changed
  • cargo fmt has been run on this branch

@felixrabe
Copy link
Copy Markdown
Contributor

felixrabe commented Jul 9, 2019

Thx for this.

You can try to work from the failing Travis builds to see if you can fix them, if they look straightforward to you.

@felixrabe
Copy link
Copy Markdown
Contributor

felixrabe commented Jul 9, 2019

Based on this output

failures:
---- event_loop_wait stdout ----
thread 'event_loop_wait' panicked at 'On macOS, `EventLoop` must be created on the main thread!', src/platform_impl/macos/event_loop.rs:45:17
---- event_loop_basic stdout ----
thread 'event_loop_basic' panicked at 'On macOS, `EventLoop` must be created on the main thread!', src/platform_impl/macos/event_loop.rs:45:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

it looks like tests are not run on the main thread. I don't know for sure, but perhaps Rust tests are run in multiple threads for speed ups. Maybe there is an option to get them to run on a single (main) thread instead?

@rikusalminen Can you insert an assertion into the tests you added to make sure they are run from the main thread? I suspect that change would cause them to fail on your system too until you figure out how to run tests single-threaded.

Curious to hear back from you how this all turns out!

@rikusalminen
Copy link
Copy Markdown
Contributor Author

Unfortunately it does not seem like there's a way to make cargo test use the main thread without some hacks (this is the best I can come up with: https://stackoverflow.com/questions/43458194/is-there-any-way-to-tell-cargo-to-run-its-tests-on-the-main-thread)

@felixrabe please advice what to try next? Make the test(s) skip on macos?

In any case, the actual fix patch can be merged (or cherry-picked) while thinking about a solution for running tests on macos.

Are there plans for creating tests that actually use the windowing API?

@rikusalminen
Copy link
Copy Markdown
Contributor Author

I also noticed that Linux travis tests will fail because no backends can be initialized.

@felixrabe
Copy link
Copy Markdown
Contributor

I suggest you revert the tests (or force-push on a branch that does not have tests ; also update the title of this PR), and open a separate PR for just the tests. Then we can merge this change (if the project owners think it is good), and discuss how to approach testing in the new PR.

Before starting the event loop, invoke callback with
NewEvents(StartCause::Init).
@rikusalminen rikusalminen changed the title Add NewEvents(Init) callback to x11, add event loop test Add NewEvents(Init) callback to x11 Jul 10, 2019
@rikusalminen
Copy link
Copy Markdown
Contributor Author

rikusalminen commented Jul 10, 2019

I have removed the test case from this pull request.

I'll consider what to do with it, maybe it could be put on the examples folder instead of automatic testing.

This PR is ready for being merged.

@Osspial
Copy link
Copy Markdown
Contributor

Osspial commented Jul 10, 2019

@rikusalminen There's been talk at various points of creating an interactive test suite since a large portion of Winit is hard to automatically test, so it may make sense to put this test in there if somebody starts work to create it.

@Osspial Osspial merged commit 5ca828d into rust-windowing:master Jul 10, 2019
Osspial added a commit to Osspial/winit that referenced this pull request Jul 11, 2019
commit 1ea29b4
Author: Riku Salminen <[email protected]>
Date:   Tue Jul 9 13:02:02 2019 +0300

    x11: NewEvents(StartCause::Init) callback at start

    Before starting the event loop, invoke callback with
    NewEvents(StartCause::Init).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants