-
Notifications
You must be signed in to change notification settings - Fork 16
fix: Exception catching #575
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
Conversation
|
So, I loaded the URL from #569 and got the error. Then I clicked on the sequence changer and got the attached screenshot. Then I clicked on A000005 and despite the thumbnail working, the visualizer gave the same error. So the thumbnail and scope contradict each other. Which is correct? Update: A clue is provided by the reproduction procedure of @Vectornaut in #537 , where opening the sequence changer and choosing A00005 works fine, then going to a sequence producing an error and back to A00005, the error stays on screen. So I think it isn't clearing. In fact, although the circle arrow doesn't do anything, a full page refresh gets the visualizer working again.
|
|
Indeed, somehow the current error catching can't recover once it's caught an error, even if the error is fixed by changing parameters. Will investigate and push a fix when I have one. |
|
Funny behaviour: Use this url: http://localhost:5051/?name=Random+integers+0+to+9&viz=Turtle&ruleMode=1&stepFormula=50*log%28a%29&seq=Random&max=19 |
It looks like our tests are running in happy-dom, so maybe we can use the happy-dom API to create an ambient page? Based on a quick skim of the happy-dom documentation, the |
This was happening because we have to recycle the WebGL contexts, so the thumbnails were each waiting a bit, snapshotting their visualizations, and then departing the visualization to release the WebGL context. It was at that depart() that the error overlay was coming down. So I changed depart to specifically not clear the error overlay, and went through the (merely) three places depart() is called and manually cleared the error overlay, except of course for that one in Thumbnail where we want it to stay. The happy-dom worked for testing the overlay functions, so that's done, too, taking care of all of the concerns voiced at the beginning of this PR or found so far. Marking as ready for review. |
|
The answer to this is probably no, so don't sweat it, but is there any way to make the text copy-paste-able? That's a feature of the best types of error messages, especially when instructed to email the error to someone. :) |
|
Maybe instead of the last line about reloading and emailing, it could say "See Documentation > Errors for more information". Then we could expand in the docs on the fact that this could be a result of some choice of parameters, give some examples of the types of things (e.g. log(0)), troubleshooting, and further instructions on how to contact us. |
|
Possible aesthetic suggestions:
|
Hmm, how far do you want to go with this? I think the answer is that the text is selectable and copyable, just it's over a canvas that is intercepting all mouse events, so none are getting to the overlay. You can verify this by (either in the Scope or in the sequence chooser modal) clicking just outside/above the visualization canvas, and dragging far enough to be below it. Voila, the entire overlay text selects, because the page has gotten those mouse events, and so it's now selecting everything you've dragged out, including the overlay. If you now do a "copy", you can paste the whole text anywhere you like. So I think to get things to work the ideal way, so that you could select/copy within any of the text of the overlay, would involve either
In other words, it seems like a fair amount of work and I am not really sure exactly how to proceed. So we could: (a) Grit our teeth, figure it out, and get it working for alpha Happy to do whichever of these, just let me know. |
OK, changed the guidance as suggested. Did you want expanding the Errors section in the User Guide to be part of this PR? |
A minimal version, yes, so at least there is an Errors section -- it's kind of broken if it points to something not existing? I think a paragraph is fine for now, but then we can expand there if/when needed. What do you think? Maybe under "User Guide" after Visualizers and Sequences, so just "User Guide > Errors"? |
Haha. :) I choose (b). One can also successfully ignore issues, so it includes (c). Although you could point out an easy cut-n-paste workaround in the "Errors" section of the docs. You can just "CTRL-A" on the webpage and then paste it into a notepad, and the error message will be included along with all the text of the control panels. (I was unable to reproduce your version of trying to grab it from outside the canvas area with the mouse, but CTRL-A worked well.) |
There already is such a section, which is why I referred to it. What aspects are missing from it that you would like to see in this PR? Thanks. |
Issue #576 filed. |
Oh, duh! Sorry. Here's some proposed text (just trying to be helpful here, modify as you see fit): If Numberscope experiences an error, you might see an alert with the error message. Sometimes this results from the parameters you've chosen for the visualization. For example, if you use a formula |
I really think that alerts need to be visually distinguished, and one important way is through color. I just switched to the only non-greyscale color in our palette, which seems fine to me. What do you think?
Filling the whole thumbnail seems really jarring to me -- that is huge! I just put it at 60px, and on its own line. That way (at least on my screen), on a thumbnail, you see a nice big danger symbol and then just a text that there was an error, and the error message. The suggestion to see the help is then only visible by clicking or scrolling that thumbnail. Again, let me know what you think. |
OK, edited all that into the existing section, let me know how it reads. |
I like the aesthetic choices now! I think your larger symbol is clearer what is going on intuitively in the thumbnails, and it isn't so much detail now. I think it could help to have a small space at the left, so the text and symbol aren't up against the very left side of the canvas. |
It's great. |
|
OK, latest two items (leaving space at left and making the documentation reference more precise) are in. |
|
Looks great and passed all tests on my machine. Merging. |
* fix: Use error overlays instead of modal alerts; avoid uncaught errors * fix: clear error overlay on reset() or depart() * chore: lint fix * fix: Don't clear away a thumbnail error overlay when done rendering * test: use fake happy-dom to test the error overlay functions * chore: lint wants a terminal comma * refactor: Adjust the guidance on an error overlay * chore: lint again * chore: error overlay layout * doc: slightly expand Errors section of User Guide * doc: wow lint is picky about markdown linebreaks * doc: revised guidance in error overlay and adjusted layout

By submitting this PR, I am indicating to the Numberscope maintainers that I have read and understood the contributing guidelines and that this PR follows those guidelines to the best of my knowledge. I have also read the pull request checklist and followed the instructions therein.
This PR attempts to catch errors in visualization
draw()calls as well assetup(), and then handles such errors by placing a semitransparent overlay above the visualization canvas with a (hopefully helpful) message, rather than modal alert popup boxes. It should be tried with the known procedures for eliciting visualization errors described in issues #569 and #537.Concerns about this PR:
hasErrorOverlayreturn the errorOverlay if one is present, and then in thedepart()function we could do something likethis.within.removeChild(errorOverlay). I just didn't bother since I don't see any consequences for not bothering, but perhaps it's just good hygeine that we should proactively do. I don't really know how to judge.Also, of course, I welcome feedback (@katestange , @Vectornaut) on how the messages look/are laid out/what their actual text is, etc. Thanks!
Resolves #537.
Resolves #569.