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

Reintroduce the loading spinner #107

Merged
merged 5 commits into from
Jun 26, 2024
Merged

Reintroduce the loading spinner #107

merged 5 commits into from
Jun 26, 2024

Conversation

sedsedus
Copy link
Contributor

@sedsedus sedsedus commented Sep 24, 2023

Context / before

  • The loading spinner (and loading text) were removed in d12c19d
  • If WebRunner::start fails nothing is shown in HTML, the user has to open the console to see if something happened (a.k.a. halting problem)

What changed / now

  • before the canvas is loaded a CSS loading spinner with "Loading..." text is displayed
  • if the loading fails, "the app has crashed" text is shown instead
    • still panic! in this case (as it did)
  • the corresponding div is removed after loading successfully completes

How does it look

image
image

What exactly changed

  • introduce dependency to web-sys
  • add the loading_text div that shows the spinner text during loading, or error if loading fails
  • access the DOM (in main.rs) after the app has been loaded, and remove the spinner, or show an error
    • if we fail to locate loading_text (like when someone removes the div from index.html) - nothing happens

Discussion

  • I wanted to avoid dependency on web-sys, but, I don't see any other simple way to access the spinner
  • Another idea I had was to do it straight from the js side (like it was before), however, I don't see a way to receive events from WebRunner
  • If this solution is undesired, I think the CSS remnants of the spinner should be removed (like lds-dual-ring)

Alternative solution

A more thorough solution (in main.rs) is master...sedsedus:eframe_template:fix-css-spinner-install-hook, where all panics are caught to show HTML error.
I encountered this myself, where I would mistype the canvas id and wait a few seconds wondering why the app doesn't load, only to discover in the console that something failed.
This is more thorough (but at the same time a little more complex / less clean due to hook insertion) because this "invalid canvas id error" wouldn't get caught by the proposal in this PR

- it wasn't shown at all after d12c19d
- now it is shown, and then hidden when loading is done
- This gives full visibility by showing up in the browser's console
@emilk
Copy link
Owner

emilk commented Sep 27, 2023

Another idea I had was to do it straight from the js side (like it was before), however, I don't see a way to receive events from WebRunner

This is what egui.rs does: https://github.com/emilk/egui/blob/dbf3405597619fb563a1d79926560c6ab3746442/docs/index.html#L132-L192

…but this PR seems a lot simpler imho. Less JavaScript is always a win in my book.

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!

Adjust the html crash message

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
@sedsedus
Copy link
Contributor Author

Less JavaScript is always a win in my book.

I like this book :).

Before merging please also consider merging (I think you should be able to) this alternative into this branch: sedsedus#1
^ is potentially better because it catches all panics (unlike this PR), but I am not sure if main.rs hasn't become too complex for a "plain" template (panic hooks)

@emilk emilk merged commit 6ec4bd5 into emilk:main Jun 26, 2024
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.

2 participants