-
Notifications
You must be signed in to change notification settings - Fork 16
fix: Ensure unmounting a thumbnail or a Scope view cleans up the visualizer #585
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
|
First, I noticed a bug in the Integersteller gallery item which I'm hoping can be rolled into this too. It's not strictly speaking a bug, but it's not the desired thing, either. If you go to Integersteller and then try to change sequence, the picture doesn't actually change for any other sequence. That's because the formula has gcd(x,y) in it instead of gcd(A(x),A(y)). They are the same on the integers, but the latter would adapt the visualization to any other sequence, which is a natural thing the user would like to do when they click "change sequence". Ooops! Can we roll it in here? |
|
Next up, here's a real bug. To reproduce:
So apparently we removed too much of something. This appears to be specific to Turtle. As an apparently separate issue, Recaman seems to fail and give a sad face (not a proper error) on the canvas in the background if you leave the change sequence thumbnail modal up for a bit and/or switch windows a bit. But this seems to happen in ui2 also. |
Not great practice but under the pressure of trying to have this ready for this upcoming week, OK, let's do it. But to be clear, the formula should become: Correct? Or do we want: so that you also see where your cursor is in addition to the sequence values? It will be redundant for Integerstellar, but could be helpful for other sequences... |
Should be fixed with the latest commit. Thanks for the great catch. |
Unable to reproduce. Please either post a very explicit recipe for causing the behavior, or we can wait 'til post-alpha. I was hoping to deploy alpha before I got on the plane, but I guess that won't be happening :-/ so I guess another alternative is you can show me :-). What I will do now is check in the second Intergerstellar patch above, that pops up both the location and the gcd, guessing that's the one we want, to try to minimize the steps until we can deploy. But if you'd rather have something different, just let me know and of course we can change it again. If you do happen to look at this again before you sleep, feel free to send feedback, if I can act before I get on the plane I will. Thanks! |
|
OK, the adjustment to the Integerstellar formula is in, the second version I posted that shows the location and the gcd. So as far as I am concerned, this is ready to merge and then ui2 is ready to go to main and alpha is ready to be deployed. So the only remaining questions:
I am standing by, will take action on any feedback as soon as I can. Sorry this came down to the wire. |
Yes, correct, the first of your two formulas seems the most correct and informative to me. |
Oh wait, I misunderstood! The second formula seems reasonable. I'll try it. |
Can't reproduce either -- maybe my browser was in a weird state?
No, looks good now.
It all looks good to me!! |
…alizer (#585) * fix: ensure that visualizations are cleaned up when they are supposed to * fix: Don't add a viewscale parameter if the thumbscale not specified * refactor: Redefine Integerstellar to better employ the selected sequence
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.
Previously, references were being held to the HTML containers for visualizers. This would hinder garbage collection and allow visualizers to reawaken and continue to draw in a "zombie" state in which they were not connected to any visible element, draining cpu resources.
The PR removes all such references, preventing frontscope from bogging down as you switch back and forth from the gallery to individual visualizers.
It also fixes a typo in the definition of VFib Snowflake, and patches a bug which allowed some FormulaGrid visualizations to draw forever, rather than stopping once they had displayed everything available.
Resolves #581.