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

Re-introduce Web examples #3637

Merged
merged 1 commit into from
Apr 18, 2024
Merged

Conversation

daxpedda
Copy link
Member

This adds back the custom cursor stuff to the examples.
The only other example I didn't port was the scale factor one, which I didn't feel make a whole lot sense after #2859.
The web example only showed events inside the window, which we don't need anymore if we print them properly in the console, which I've done now.

As part of showing thing properly in the console, I've switched to using tracing instead of println! in the relevant examples.

I didn't find any mention of how to run examples on any target, e.g. Android or iOS.
So I feel we either have to add docs for all of these targets or none. Ergo I'm also tending towards removing our run-wasm dependency.
WDYT?

Fixes #3473.

@daxpedda daxpedda added this to the Version 0.30.0 milestone Apr 18, 2024
examples/window.rs Outdated Show resolved Hide resolved
examples/window.rs Outdated Show resolved Hide resolved
custom_cursors[1].clone(),
event_loop.create_custom_cursor(CustomCursor::from_url(
format!(
"https://picsum.photos/128?random={}",
Copy link
Member

Choose a reason for hiding this comment

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

Could we use url to github for that? Like we have cursors laying around in the repo already in png formats, so they should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well the point is to always have it be a different picture, so you can see what happens when its not loading immediately.

run-wasm/src/main.rs Outdated Show resolved Hide resolved
examples/window.rs Outdated Show resolved Hide resolved
examples/window.rs Show resolved Hide resolved
examples/window.rs Outdated Show resolved Hide resolved
examples/util/tracing.rs Outdated Show resolved Hide resolved
examples/window.rs Outdated Show resolved Hide resolved
examples/window.rs Outdated Show resolved Hide resolved
@daxpedda daxpedda force-pushed the web-examples branch 3 times, most recently from bded977 to f011577 Compare April 18, 2024 15:27
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I'm also tending towards removing our run-wasm dependency.

CC @rukai

@daxpedda
Copy link
Member Author

I'm also tending towards removing our run-wasm dependency.

I just went ahead and did that, we can consider re-introducing it if we decide we want it after all.

@daxpedda daxpedda merged commit c15fa6e into rust-windowing:master Apr 18, 2024
52 checks passed
@rukai
Copy link
Contributor

rukai commented Apr 18, 2024

For the web example drawing events to the dom: that was valuable for testing the examples on mobile devices which do not have a console accessible in their browsers.

For run-wasm, if there's an alternative way you want users to run wasm examples then by all means swap to that. But currently there is no documented way to run wasm examples so I'm quite confused.
This all or nothing approach to documentation is really strange, it doesn't have to be perfect but surely we should support the major platforms well with documentation?

@daxpedda
Copy link
Member Author

For the web example drawing events to the dom: that was valuable for testing the examples on mobile devices which do not have a console accessible in their browsers.

Good point!

For run-wasm, if there's an alternative way you want users to run wasm examples then by all means swap to that. But currently there is no documented way to run wasm examples so I'm quite confused.
This all or nothing approach to documentation is really strange, it doesn't have to be perfect but surely we should support the major platforms well with documentation?

My original comment:

So I feel we either have to add docs for all of these targets or none. Ergo I'm also tending towards removing our run-wasm dependency.

Agreed, this is stupid. I think this was just my first thought: if I'm gonna start adding documentation, where is the documentation for the other targets?

run-wasm is not required to run the Wasm examples, there are plenty of Rust Wasm runners out there that can be configured globally, e.g. I'm using wasm-server-runner. So I don't need any additional work at my local Winit repo to run the examples.

I'm aware of the advantages run-wasm offers in comparison to other runners and I don't think Winit really needs them. This is why I decided to remove run-wasm, in addition to it not being documented.

Though I'm happy to re-add them upon popular request.

Apologies for not originally stating my full reasons in the OP.

@rukai
Copy link
Contributor

rukai commented Apr 18, 2024

All good.
I agree that cargo-run-wasm requires a certain level of integration with the repo that isn't ideal. I think the solution here is to just point users towards wasm-server-runner in the readme.

Edit: or probably just a readme specific to the examples directory

@daxpedda
Copy link
Member Author

Would stick with keeping it in the README, pending input from the other maintainers.

Happy to look at a PR!

@madsmtm
Copy link
Member

madsmtm commented Apr 18, 2024

I think I'd prefer it in winit::platform::web, and then perhaps instead link more prominently to that from lib.rs

@daxpedda
Copy link
Member Author

I think I'd prefer it in winit::platform::web, and then perhaps instead link more prominently to that from lib.rs

I think it definitely needs something visible from the repo, when you clone the repo you don't automatically run cargo doc, where usually "repo documentation" isn't found.

@madsmtm
Copy link
Member

madsmtm commented Apr 18, 2024

I'd also be fine with a examples/README.md with instructions specific to the examples

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Merge web example into window example
4 participants