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

On wasm, provide intradoc-link for spawn() function in EventLoop docs #3178

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 commented Oct 24, 2023

For 0d366ff#r130681051

And on any other platform, emit a footnote explaining that this function is only available on wasm.

EDIT: Apologies, it looks like gh pr create pushed this through the main repo instead of my fork. Hence there's no marijn/ prefix on the branchname.

MarijnS95 referenced this pull request Oct 24, 2023
This re-works the portable `run()` API that consumes the `EventLoop` and
runs the loop on the calling thread until the app exits.

This can be supported across _all_ platforms and compared to the
previous `run() -> !` API is now able to return a `Result` status on all
platforms except iOS and Web. Fixes: #2709

By moving away from `run() -> !` we stop calling `std::process::exit()`
internally as a means to kill the process without returning which means
it's possible to return an exit status and applications can return from
their `main()` function normally.

This also fixes Android support where an Activity runs in a thread but
we can't assume to have full ownership of the process (other services
could be running in separate threads).

Additionally all examples have generally been updated so that `main()`
returns a `Result` from `run()`

Fixes: #2709
)]
///
/// [`set_control_flow()`]: EventLoopWindowTarget::set_control_flow()
/// [`run()`]: Self::run()
Copy link
Member Author

@MarijnS95 MarijnS95 Oct 24, 2023

Choose a reason for hiding this comment

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

Not super happy with this, because it'll fail to link on not(all(wasm_platform, target_feature = "exception-handling")) (which is strange, because features must be additive and should not remove functionality).

Copy link
Member

Choose a reason for hiding this comment

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

target_feature = "exception-handling" isn't stable, which is probably why it didn't work for you.

Copy link
Member Author

Choose a reason for hiding this comment

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

@daxpedda reading this again, I missed target_ and assumed it was just feature = (from the [features] list in Cargo.toml).

I think the feature must have been _en_abled (running nightly rustdoc for --cfg docsrs => doc_cfg), as it's the only way to cause not(all(true, true)) = not(true) = false and turn off fn run()?

Copy link
Member

Choose a reason for hiding this comment

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

That is weird, target_feature = "exception-handling" should not automatically have been enabled by running nightly rustdoc.

//! [`DeviceEvent`]: event::DeviceEvent
//! [`UserEvent`]: event::Event::UserEvent
//! [`LoopExiting`]: event::Event::LoopExiting
//! [`Event::LoopExiting`]: event::Event::LoopExiting
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want this PR to turn into a "let's fix all winit intradoc links", but a few more changes are still needed to make this consistent.

No clue what the point is with manual relative .html links that may not resolve when certain features are not turned on... Pointing the user to a 404 page :(

Copy link
Member

Choose a reason for hiding this comment

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

This is probably a problem for another time, but these methods are deprecated anyway.

src/event_loop.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/platform/web.rs Outdated Show resolved Hide resolved
@MarijnS95 MarijnS95 force-pushed the wasm-link-to-spawn branch 2 times, most recently from 3627974 to 514564b Compare October 25, 2023 13:01
…docs

And on any other platform, emit a footnote explaining that this function
is only available on wasm.
@daxpedda
Copy link
Member

Just rebased so we can merge.

@daxpedda daxpedda merged commit c235bd1 into master Oct 25, 2023
50 checks passed
@daxpedda daxpedda deleted the wasm-link-to-spawn branch October 25, 2023 15:42
@MarijnS95 MarijnS95 added the S - docs Awareness, docs, examples, etc. label Oct 25, 2023
kchibisov pushed a commit to kchibisov/winit that referenced this pull request Oct 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S - docs Awareness, docs, examples, etc.
Development

Successfully merging this pull request may close these issues.

None yet

2 participants