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

Fix exception when WGLMakie is initalised after element removal #4343

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

frankier
Copy link
Contributor

Description

Fixes #4342

This simple fix just checks if the element is still there before trying to initalise the canvas and bails if it's not there.

I can add a test/changelog entry if/when you agree this approach is ok.

Type of change

Delete options that do not apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Added an entry in CHANGELOG.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@SimonDanisch
Copy link
Member

Sounds like a good idea!
It should not just return though, but instead push a value to init and send an error:

} catch (e) {
Bonito.Connection.send_error("error initializing scene", e)
$(done_init).notify(false)
return
}

I actually just started pushing the error to done_init in: https://github.com/MakieOrg/Makie.jl/pull/4317/files#diff-468ae987abf57692bfa58a73c15591f64a022fba56dd8a4b7fdee38d6a392970
So, after that got merged, it should likely also push a good error instead ;)

@frankier
Copy link
Contributor Author

frankier commented Sep 11, 2024

That wasn't quite the situation I was thinking of, although I can see why you might want an exception in some cases.

I'm starting to look at having a Bonito app/WGLMakie plot as a widget/component within a larger page. We could for example plot shown in a modal element that the user immediately closes before WGLMakie initializes. If we are to have "widget semantics" it should be that removing an element tears down all associated state and exits cleanly. I have another issue coming (still getting a MWE) to do with a different problem when an element is replaced but where I'm also assuming Bonito will work with "widget semantics".

In case you don't see this as a desirable default, maybe Bonito could have a "widget semantics" mode separately. Or perhaps it's a case of distinguishing between when the whole Bonito root was removed from some other case? (What other case though?) It would be possible to make this distinction using disconnectedCallback(), but this would mean everything would need to be wrapped in a web component.

@SimonDanisch
Copy link
Member

I think this fix is fine, if it solves your problems ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Work in progress
Development

Successfully merging this pull request may close these issues.

Exception thrown when WGLMakie containing element is removed
2 participants