Update eframe, add trunk setup #11
Conversation
| if let Some(web_info) = &frame.info().web_info { | ||
| // allow `?url=` query param | ||
| if let Some(url) = web_info.location.query_map.get("url") { | ||
| self.url = url.clone(); |
There was a problem hiding this comment.
This previous logic looked into frame to figure out whether we are in a web environment or not. However, with the use of the new() method to instantiate this app, it doesn't seem to have access to the frame information. Because of this, I'm not sure how to go about getting the information from before.
I know update gives us access to the frame, but I don't imagine putting this logic in there to be run every frame would be the best.
How might this logic be migrated the best?
| // allow `?url=` query param | ||
| if let Some(url) = web_info.location.query_map.get("url") { | ||
| self.url = url.clone(); | ||
| fn connect(&mut self, ctx: Context) { |
There was a problem hiding this comment.
I've let this function take a moved Context, and everywhere that calls this function clones Context first. Is that the right way to handle passing context? If I just pass a reference, then I run into an issue with lifetimes that I didn't want to explore.
Is Context designed to be cloned?
| #[cfg(not(target_arch = "wasm32"))] | ||
| frame.close(); |
There was a problem hiding this comment.
I believe this is the best way to handle the case where we're not in WASM? Maybe it would be better to just have this entire TopBottomPanel be scoped and cfgd?
| { | ||
| self.connect(frame.clone()); | ||
| self.connect(ctx.clone()); |
| pub fn start(canvas_id: &str) -> std::result::Result<(), eframe::wasm_bindgen::JsValue> { | ||
| pub fn start(canvas_id: &str) { |
There was a problem hiding this comment.
Another area I'm unsure about. We seem to have two entrypoints for WASM; the one that is added in this commit, and the existing one in lib.rs. Should only one be required?
Also, removing the return type is just to get it to compile. Not sure if this is the right thing to do here just yet.
|
awesome to see you used my issue & commit :) good luck with the PR! |
|
Hi @AngelOnFira is this going to be merged ? |
|
This has been in draft for too long. Please try to make your PRs small and focused. If you want to update eframe or add trunk, please do so in separate PRs |
This takes the work started in #9, and updates eframe.
I'm going to start this as a draft PR, since although everything compiles, there are still some areas that I need to fix, and have questions about.
The biggest item is migrating the use of the
Apptrait; previously, thesetupfunction was used to prepare the websocket listener. Some of the logic in this PR might not be feature similar just yet.This will close #9.