Closed gwhitney closed 3 weeks ago
Looks like I need to update the Github CI test snapshots, so marking this draft until I do. I will also check in instructions for how to update those snapshots, into doc/code-tests.md
OK, detailed instructions as well as new snapshots in place. Ready for review.
Bug: Try this url: http://localhost:5173/?name=ModFill&viz=ModFill&modDimension=1000&seq=Formula&formula=n%21 and then try to change the mod dimension in modfill. It reinitializes but the mod dimension drawn doesn't change. However the parameter box and url changes. A hard browser reset or the soft header bar reset will result in the modfill being actually changed in the visualizer as expected.
Review question: Is it ok if I just trust you on the CLI snapshot instructions until next time I'm doing a visualizer overhaul and have to do that myself (which will definitely happen before alpha)?
Review question: Is it ok if I just trust you on the CLI snapshot instructions until next time I'm doing a visualizer overhaul and have to do that myself (which will definitely happen before alpha)?
Well, I wrote down exactly what I did, so it definitely works for me, so sure I don't have any issue putting off verifying it works the same way for someone else.
[looking into the other bug you found, thanks]
It reinitializes but the mod dimension drawn doesn't change. However the parameter box and url changes.
Good catch. I got overly ambitious with how much stuff I put in presketch -- I got confused about how presketch() doesn't re-run on typical parameter changes but setup() does, which is on purpose so you can control what gets reset and what doesn't. The framework is still working as expected, I just arranged the initializations wrong when I refactored ModFill. Should be working now.
These facilities are also used to implement a proper warning in ModFill for Mod Dimension being too large.
I get a warning like "Running with maximum modulus 14176, since 600000 will not fit on screen." But when I resize, the number 14176 doesn't change. Only a reset (browser or header button) after resizing will cause the error number to change. Also, it seems to cap out at 14176 and even when I resize larger & reload it doesn't update this number. I thought it would be the canvas width?
Good catch. I got overly ambitious with how much stuff I put in presketch
Works now, thanks.
Also, it seems to cap out at 14176 and even when I resize larger & reload it doesn't update this number. I thought it would be the canvas width?
Oh I think I see why this happens. The canvas wants the line of slope one to go corner-to-corner so the boxes are actually rectangles. After a certain point in my resize, my height has reached max, and this artificially caps the width and just stretches the boxes. Which is to say, that's totally fine.
But when I resize, the number 14176 doesn't change. Only a reset (browser or header button) after resizing will cause the error number to change.
Yup, you've found another problem. Looking into it.
OK, here's my take on this: presketch() explicitly takes a size argument that gives what the size of the canvas is going to be. That implies to me that if the size was going to be something different, then the framework would be obliged to call presketch again, because maybe I made some decision based on the size, that I would then need to adjust for a different size. (In fact, that's exactly what was biting ModFill.) But the framework was not necessarily calling presketch again when the size changed; sometimes it would, sometimes it wouldn't. So I changed the code to make sure that the framework calls presketch() again when the size changes. If that all seems appropriate to you, we should be good to go for further review. Your review so far has been super productive, thanks!
If that all seems appropriate to you, we should be good to go for further review. Your review so far has been super productive, thanks!
That sounds good to me. Should it be clarified in the "make a visualizer" doc?
OK, I believe I have made all of the requested changes. I expect that the CI tests will fail because the NumberGlyph snapshot has changed. I will make another commit when the CI tests finish, if so. But it should be fine to continue your review any time.
Looking good to me! There's only a typo above, and whether you want to add a little something to the documentation.
OK, how's that?
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 puts new properties in every Paramable object, representing the ValidationStatus of every parameter and the overall ValidationStatus of the Paramable as a whole. These statuses are then used directly in the user interface. These changes allow Sequences and/or Visualizers to
(a) Make sure that their messages show up in the desired place: at the top of the tab for the object, or adjacent to the proper parameter entry field; and (b) Update warnings at any time during operation, not just during checkParameters().
These facilities are also used to implement a proper warning in ModFill for Mod Dimension being too large. The location of errors and warnings are improved throughout the visualizers, and in the Formula sequence.
Resolves #112. Resolves #113.