numberscope / frontscope

Numberscope's front end and user interface: responsible for specifying sequences and defining and displaying visualizers
MIT License
7 stars 14 forks source link

Dynamic URLs #354

Closed kaldis-berzins closed 1 week ago

kaldis-berzins commented 1 week ago

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 is first in the PR merge chain.

URLs

Specimen

gwhitney commented 1 week ago

Thanks for submitting these PRs and clearly marking the desired merge order. I will start by reviewing this one. If we don't get through the usual back-and-forth of review for the end of the semester, we will take it from whatever point we do reach.

But one initial question about this PR: a good fraction of the changes consist of changing the export modules of sequences and visualizers to get their names and descriptions from constants in their source files, rather than from properties of the classes that define them, roughly reverting a change from the prior PR. It would help us to understand: (a) What is the motivation for these changes? (b) Are they needed for the URL encoding/decoding of Specimen parameters, or is it a separate change that happened to be included in this PR?

gwhitney commented 1 week ago

(a) What is the motivation for these changes?

Ah, I think I understand now. I think it is because the approach from the previous PR actually simply didn't work, even though it typechecked and compiled and seemed to run. I will continue the review on that basis, although I am unfortunately out of time for tonight.

gwhitney commented 1 week ago

OK, I have found the URLs to update and load correctly across some different Visualizers and Sequences. I could not "trick" the system into producing a bad URL by e.g. entering a formula that would not parse and then copying the URL. This seems to work well and be reasonably limited in scope. So I am comfortable with merging it as soon as the above code review comments are addressed. I am willing to add a commit or two that addresses them myself in the interest of expediency, if that seems reasonable to either @katestange or @Vectornaut, so please just let me know (or the team can just go ahead and address them if someone has time). Thanks!

katestange commented 1 week ago

OK, I have found the URLs to update and load correctly across some different Visualizers and Sequences. I could not "trick" the system into producing a bad URL by e.g. entering a formula that would not parse and then copying the URL. This seems to work well and be reasonably limited in scope. So I am comfortable with merging it as soon as the above code review comments are addressed. I am willing to add a commit or two that addresses them myself in the interest of expediency, if that seems reasonable to either @katestange or @Vectornaut, so please just let me know (or the team can just go ahead and address them if someone has time). Thanks!

I would say go for it @gwhitney , that way if something more important to contact the delft team about lies in wait in a later PR, we are more likely to get to it while we still have some contact with them. That was a complicated sentence, sorry. Also, you have all the changes fresh in mind and they look fairly simple (merge errors etc).

gwhitney commented 1 week ago

OK, in progress, one small commit out of roughly 3 projected.

gwhitney commented 1 week ago

OK, have made all of the suggested changes. Merging.