Wasm Improvements - Githubissues

emoon / rust_minifb

Cross platfrom window and framebuffer crate for Rust

MIT License

1.04k stars 99 forks source link

Wasm Improvements #363

Closed augustkline closed 1 month ago

augustkline commented 2 months ago

This PR makes some changes to how Windows are instantiated on wasm targets

The title parameter in Window::new() has been replaced with container, which takes an element the canvas should be appended to
- Allows more control over where and how to embed a minifb canvas in web projects, and still allows the user to change the window title with set_title
- Rustdoc new window example has been updated for wasm32 target
set_cursor_visibility now properly sets the the html canvas cursor style
is_active now checks if the canvas element is focused

augustkline commented 2 months ago

To avoid a breaking API change, title could be used to select a dom element by id...

emoon commented 2 months ago

Hey,

Thanks for the PR! I wonder if it's possible to include an example that works with these changes also? It would be nice to keep the API same between desktop and wasm, but if it doesn't make sense from a web perspective I'm fine with the change.

augustkline commented 1 month ago

Hi! Sorry for the delay! Working on the example now, and pretty quickly I'm running into a case where having a unified API would be better: all non-web examples fail to compile when the target is set to wasm... I do think that on web the page title shouldn't be changed, since most of the time (i assume) someone would be embedding minifb into a larger project/site where the page title is already defined somewhere else. I think passing the id of a dom element works, as long as there's some sign (e.g. rustdoc comment) that the behavior is different between desktop & web

augustkline commented 1 month ago

Also, I noticed that the web feature is only used in 2 places (rate.rs and key_handler.rs) and could probably be replaced with a target = "wasm32" attribute, would it make sense to replace those and remove the web feature?

emoon commented 1 month ago

Also, I noticed that the web feature is only used in 2 places (rate.rs and key_handler.rs) and could probably be replaced with a target = "wasm32" attribute, would it make sense to replace those and remove the web feature?

Sounds good to me

augustkline commented 1 month ago

This should be ready to go!

emoon commented 1 month ago

Great! Thanks.

I wonder how hard would it be to add a build step for this in https://github.com/emoon/rust_minifb/blob/master/.github/workflows/ci.yml ? Just to make sure it doesn't break when doing other changes.

augustkline commented 1 month ago

I think this change should work... Building is no problem, but testing on wasm requires a whole bunch of extra tooling since the generated wasm isn't an executable on its own. Since the only tests being run anyways are doc tests this should be fine:

    - name: Build
-     run: cargo build --verbose
+     run: |
+          cargo build --verbose
+          cargo build --verbose --target wasm32-unknown-unknown 
    - name: Run tests
-     run: cargo test --verbose
+     run: |
+          cargo test --verbose
+          cargo test --doc --verbose --target wasm32-unknown-unknown

more info on wasm testing here

emoon commented 1 month ago

Thanks for the PR. I will look into getting the build step for the CI running in another change.