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

Graceful exit from web #1650

Merged
merged 5 commits into from
Aug 2, 2022
Merged

Graceful exit from web #1650

merged 5 commits into from
Aug 2, 2022

Conversation

enomado
Copy link
Contributor

@enomado enomado commented May 20, 2022

  • holds all event-handles and gracefully unsubscribes them.
  • destroys GL painter.
  • tries to resize canvas to previous container's size, not screen size (it should be 100% height thought)

With this PR egui could be running as a part of web-app and stop and start again without resource leaks(I hope)

Fixes #1642

⚠️ IMPORTANT!

You need to update your CSS with html, body: { height: 100%; width: 100%; }!

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Did a quick read-through. Thanks for working on this!

eframe/src/lib.rs Outdated Show resolved Hide resolved
eframe/src/lib.rs Outdated Show resolved Hide resolved
eframe/src/lib.rs Outdated Show resolved Hide resolved
eframe/src/web/backend.rs Outdated Show resolved Hide resolved
eframe/src/web/backend.rs Outdated Show resolved Hide resolved
eframe/src/web/mod.rs Show resolved Hide resolved
eframe/src/web/backend.rs Outdated Show resolved Hide resolved
eframe/src/lib.rs Outdated Show resolved Hide resolved
eframe/src/lib.rs Outdated Show resolved Hide resolved
eframe/src/lib.rs Outdated Show resolved Hide resolved
@enomado enomado requested a review from emilk May 21, 2022 13:10
@emilk
Copy link
Owner

emilk commented May 29, 2022

Did you try returning the handle from start_web? I think it would be a much nicer design (since it can support multiple apps in the same WASM, and since global mutable state in general is not very idiomatic Rust), but I can understand if you feel it is outside the scope of this PR.

@enomado
Copy link
Contributor Author

enomado commented May 29, 2022

That was planned at first, but I was not sure I understand how it will be managed by garbage collector, but looks like its fine. I'll do it, but cant find time for now.

@enomado enomado force-pushed the wasm_graceful_exit branch 2 times, most recently from 714e3d1 to 58c88ca Compare July 27, 2022 10:37
@enomado
Copy link
Contributor Author

enomado commented Jul 27, 2022

I've made this dont use global variable

eframe/src/web/backend.rs Outdated Show resolved Hide resolved
eframe/src/web/backend.rs Outdated Show resolved Hide resolved
eframe/src/web/events.rs Outdated Show resolved Hide resolved
eframe/src/web/backend.rs Outdated Show resolved Hide resolved
eframe/src/web/mod.rs Show resolved Hide resolved
Comment on lines +33 to +35
pub struct WebHandle {
handle: Arc<Mutex<AppRunner>>,
}
Copy link
Owner

@emilk emilk Jul 29, 2022

Choose a reason for hiding this comment

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

This WebHandle wrapper should be moved into eframe instead so that all eframe users can stop their instances!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I tried to make egui embeddable, not just run in whole document. WebHandle is extensible. You have handle in js and you can call some methods to interact with egui app

Copy link
Owner

Choose a reason for hiding this comment

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

Sure, but what is the point of defining WebHandle in the example app? I don't see stop_web() being used anywhere. Is it dead code? Should docs/index.html store the handle as a demonstration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well. Ok. I'll create another wrapper in eframe

@emilk
Copy link
Owner

emilk commented Jul 29, 2022

I've made this dont use global variable

Thanks ❤️

@enomado
Copy link
Contributor Author

enomado commented Jul 29, 2022

Btw, what do you think on adding fn as_any_mut(&mut self) -> &mut dyn Any; in App trait?

I need it to get original App from WebHandle

@enomado enomado force-pushed the wasm_graceful_exit branch 2 times, most recently from 3814ad1 to 4d0957b Compare July 29, 2022 14:17
@enomado
Copy link
Contributor Author

enomado commented Jul 30, 2022

Fixed Web, IntervalHandle, looks like most of issues. Quick checked and seems everything is fine

@enomado
Copy link
Contributor Author

enomado commented Jul 30, 2022

squashed and rebased

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Looks good! Still needs a line in eframe/CHANGELOG.md noting the breaking changes (in particular the need to add body { height: 100%; width: 100% } in the CSS).

All lines in the changelog should link to the relevant PR, and so it would be great if the PR description could be updated with:

A) breaking changes (things all users must do when updating eframe)
B) a short summary or example of what the new features enable (having two egui instances in the same web page), and how to achieve it (does the user need need to call stop_web form JS when deleting a canvas with eframe running in it?)

docs/index.html Show resolved Hide resolved
eframe/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +33 to +35
pub struct WebHandle {
handle: Arc<Mutex<AppRunner>>,
}
Copy link
Owner

Choose a reason for hiding this comment

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

Sure, but what is the point of defining WebHandle in the example app? I don't see stop_web() being used anywhere. Is it dead code? Should docs/index.html store the handle as a demonstration?

@emilk
Copy link
Owner

emilk commented Jul 30, 2022

squashed and rebased

thanks for the rebase, but please don't squash - it is easier to follow changes via commit history. I always squash when merging anyway, so the history will still end up clean!

Co-authored-by: Emil Ernerfeldt <[email protected]>
@enomado
Copy link
Contributor Author

enomado commented Jul 30, 2022

thanks for the rebase, but please don't squash - it is easier to follow changes via commit history. I always squash when merging anyway, so the history will still end up clean!

got it

@enomado
Copy link
Contributor Author

enomado commented Jul 30, 2022

I don't see stop_web() being used anywhere. Is it dead code? Should docs/index.html store the handle as a demonstration?

it works in scenario where egui app is just a canvas in normal web page. I'll create another page

@emilk
Copy link
Owner

emilk commented Jul 30, 2022

it works in scenario where egui app is just a canvas in normal web page. I'll create another page

Well, that's exactly what the web-demo in index.html is too, just that the canvas fills the screen.

@enomado
Copy link
Contributor Author

enomado commented Jul 30, 2022

Well, that's exactly what the web-demo in index.html is too, just that the canvas fills the screen.

I mean we need html with some button to stop app

@enomado
Copy link
Contributor Author

enomado commented Jul 30, 2022

Added page with stop button on http://127.0.0.1:8888/multiple_contexts.html


</html>

<!-- Powered by egui: https://github.com/emilk/egui/ -->
Copy link
Owner

Choose a reason for hiding this comment

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

The example is not very pretty, but it works for now :)

Screen Shot 2022-08-02 at 17 31 04

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

@emilk emilk merged commit 64496ca into emilk:master Aug 2, 2022
emilk added a commit that referenced this pull request Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Work in html container and graceful-stop
3 participants