Tabs and reactive visualizer and sequence settings

[kaldis-berzins]

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 has two parts - tabs and reactive settings. Unfortunately we cannot disentangle the two.

Tabs

Overview

• There are two tabs, one for the visualizer and one for the sequence settings
• They are snappable to one of four corners of the screen
• They are draggable, resizable, and scroll internally.
• Buttons for manipulating the tabs aren't implemented yet but are coming later
• The specimen viewer reacts to screen size changes, but doesn't work lower than around 700px in screen width.
  • Mobile support is still in the works

    Code

• components/Tab.vue for the tab component
• views/Scope.vue for the main specimen editor. This file is rather large, but it encompasses only the tab management code, so I thought it would be wise to keep it in one place.

Testing

• Open up the page to immediately see the specimen editor
• You should be able to drag tabs out of their positions
• When both are dragged out the sidebar disappears
• Dragging them to the sides should show blue guides for snapping them back
• They should be snappable in any configuration
• Resizing the tabs should work up to a minimum size
• When snapping the tabs to a side they should expand

Real-Time Parameters

API Changes

• The value property within a parameter may be any type like before, but will likely be transformed into a string by the parameter system to accommodate for invalid inputs.
• The forceType property is renamed to type and is now mandatory, with an enum type to make it clear which types the parameter system supports.
• Parameters can now have individual validators with the validate property. Independent parameter constraints should now be placed in these functions and are distinctly separate from interdependent parameter constraints (i.e. the relationship between two or more parameters) which remains in the standard Paramable validate function.

  Real-Time Updates

• Visualizers are now re-rendered in real time when parameter values are updated. Parameters are only applied when in a valid state, so it is guaranteed that visualizers will always receive a valid set of parameters as per the validation functions.
• Visualizers can handle their own parameter changes by overriding parameterChanged, and P5 by default handles this with a hard reset.
• Visualizers perform a hard reset when parameters of the sequence are updated.
• Appropriate error messages are displayed in the UI, with their location being in respect to the problematic parameter, when constraints are violated. This indicates to the user that the new parameter value is not being sent to the visualizer.

  Code

  The majority of the code changes take place in Paramable.ts, ParamType.ts and the new Vue components for parameters: ParamEditor and ParamField.

Testing

• You should be able to change parameters in both the visualizer and sequence and see the changes updated in the visualizer instantly
• Changing the parameters to invalid values should show an appropriate error message
• You can change the visualizer / sequence in the code around views/Scope.vue:143
  • You will need to change the import to get a different sequence, but changing the visualizer should be as easy as changing the new vizMODULES['ModFill'] section.

Specimens

A new data structure exists for storing specimens, which consists of a parameterized sequence and visualizer. This class, found in Specimen.ts, automatically handles things like applying the sequence to the visualizer and viewing it in a specified HTML element. It will also support encoding and decoding from URLs, but this is something which will be tackled in the next sprint.

[gwhitney]

With the merge of #338, this PR is now conflicted, so I am marking it as draft. Please resolve the conflicts and when it is again ready for review, remove the draft marking. Thanks!

[maxd0328]

Thank you for your comments, you make very good points and most of these should be manageable. However, before we go and make the appropriate changes, I wanted to address the part about parameters being untyped ('unknown' in most cases).

This was indeed a tough decision to make, but it ultimately came down to the accommodation of invalid inputs, now that parameter updates are real-time. As you saw, the 'value' field of parameters is almost guaranteed to be changed by the system into a 'string', making it an unrealized value in a sense (not one of the appropriate type that the visualizer can work with). The only reason the type is 'unknown' rather than 'string' is so that visualizers can put any value they'd like in there without worrying about converting it to a string, but if you think it'd be cleaner, enforcing a string type works just fine as well, just a bit more annoying for visualizer writers.

The reason for this string-ification is that now the 'value' property holds whatever the user typed in, be it valid or invalid, and only once it is guaranteed that the set of parameters are valid for the visualizer (i.e. passed all validation) are the parameter values copied into top-level properties as the appropriate types. In that sense, the types of the parameters reflected in the 'value' property aren't known by typescript, and when it comes to lumping multiple parameters of a generic Paramable objects into a single map, typescript's type system is limited and leaves us with options like '{[key: string]: unknown}' to hold their values. As far as I know, in cases like these, typescript simply isn't expressive enough for us to know the type of every parameter pre-validation at compile time. The only solution I can think of is a hack where each visualizer defines an interface for its parameters (and their types), which is sent up to parameter validation code, but even then (since this code is generic over any Paramable), this interface would be represented as '{[key: string]: unknown}'.

Alternatively, the validation functions could be altered to take all parameter values as string (so pre-realization), but then these functions would have to manually realize the values, and while this would better achieve type safety, it is an extraordinarily cumbersome thing for the visualizers to have to do. Additionally, performing the validation on the top-level properties (which have types) is impossible because this system promises that top-level properties will be valid at all times according to input checks and the validation functions.

Everything mentioned above was a part of my thought process while writing this code, and how I eventually landed on the decisions that I did. In my experience, I've always found that when working with really type-generic systems like this one, where computations are done on an enumeration of generic types, that the expressiveness of the type system can sometimes get in the way. I'm happy to discuss further, as every solution is a trade-off and I understand that your priorities may not align with some of the assumptions I made.

[gwhitney]

I agree the thorniest issue of the review is typing the slightly revised Paramable interface. Nevertheless, since we are explicitly specifying a property out of a fixed collection of constants from which the types of the realized value and the validation interface are deducible, typescript should be able to properly type all of the relevant methods. I will create a mockup of how this might work on the typescript playground and post a link.

[gwhitney]

OK, it did require some definite TypeScript type gymnastics, but I have a prototype working in the TS Playground that I believe has all of the desired features: There is a static object that serves as a "parameter description" -- the single source of truth for the parameters of a given concrete paramable class. When such a class is defined based on a particular parameter description, TypeScript automatically knows it has the properties listed there (without having to repeat the list of them in the class definition), and it knows their types, and it initializes them to the given properly-typed default values. I think you shouldn't have too much trouble adapting the prototype to the specifics of the code in this PR and getting it all compiling and running, but please feel free to discuss/ask for help/etc here. Thanks for straightening this out!

[gwhitney]

Looking at that prototype I posted in my last comment, there are a number of rough edges. Number 2 below must be cleaned up; the others you may wish to:

1. I used the terms "raw" and "cooked" for the string and converted typed values of parameters, respectively. You may decide to use slightly more formal terms ;-) In any case, please just be consistent so it's clear to the code reader which is being dealt with at each point.

2. In the constructor for GenericParamable, building the default parameter values is awkward, and it used .toString() where it should have been using derealize() from the typeFunctions. This can be partly cleaned up with a helper function, but I could not find a way to get TypeScript to realize we were selecting the type-appropriate "derealize" option, so I just brute-forced it.

   
   // Tell TypeScript more about what Object.fromEntries does
   interface ObjectConstructor {
   fromEntries<T extends readonly [PropertyKey, any]>(
   entries: Iterable<T>
   ): { [K in T[0]]: T[1] };
   }

// Helper functions function defaultRaw(desc: PD): RawParamValues { const params: (keyof PD)[] = Object.keys(desc) const raw: RawParamValues = Object.fromEntries(params.map(param => [param, ''])) for (const param of params) { const typeFuncs = typeFunctions[desc[param].type] as ParamTypeFunctions raw[param] = typeFuncs.derealize(desc[param].default) } return raw }

// ... and then in GenericParamable: constructor(desc: PD) { this.savedDesc = desc this.rawParams = defaultRaw(desc) }

3. In validate() in the same class, building the "trial" converted params is similarly awkward.
Again, I have not figured out a complete way to coax the TypeScript compiler into realizing that if we use the proper .realize() call corresponding to each parameter on a string, it assembles into the desired object. Moving the guts into a helper, the best I could do is:

// This checks that only appropriate params are being used, but still has one "as" that casts the // param value types as desired (although they already are, by the type of typeFunctions) function cook(desc: PD, raw: RawParamValues) : ParamValues { const params: (keyof PD)[] = Object.keys(desc) const cooked: Record<keyof PD, any> = Object.fromEntries(params.map(p => [p, undefined])) for (const param of params) cooked[param] = typeFunctions[desc[param].type].realize(raw[param]) return cooked as ParamValues }

// .. and then in GenericParamable validate() { return this.checkParameters(cook(this.savedDesc, this.rawParams)) }

4. As already mentioned in a comment, it would be nice to see if checkParameters() in that same class can simply be left abstract. A throwing method that must be overridden is ugly.

5. TypeScript is circumvented in assignParameters() in that same class.
We can eliminate the ugliness here by reusing the helper from point (3), better sharing code:

assignParameters() {
    Object.assign(this, cook(this.savedDesc, this.rawParams))
}

6. And also in refreshParams(). (Also the naming inconsistency (Parameters vs Params) should be resolved.)
For this one, I needed a slightly different helper than number (2), but with the same type forcing. It is a shame it shares so much code with `defaultRaw()`, perhaps you can find a better factoring to improve code re-use rather than repetition: (The entire function differs only in the argument to `derealize()`.)

// Extract raw parameters that are present in cooked form: function extractRaw(desc: PD, from: Record<PropertyKey, any>) : RawParamValues { const params: (keyof PD)[] = Object.keys(desc) const raw: RawParamValues = Object.fromEntries(params.map(param => [param, ''])) for (const param of params) { const typeFuncs = typeFunctions[desc[param].type] as ParamTypeFunctions raw[param] = typeFuncs.derealize(from[param]) } return raw } // ... and then in GenericParamable refreshParams() { this.rawParams = extractRaw(this.savedDesc, this) }

8. In function P5Visualizer(PD), `Record<string, any>` is un-TypeScript-ish.
This can be fixed with Object.fromEntries:

function P5Visualizer(desc: PD) { const defaultObject = Object.fromEntries(Object.keys(desc).map( param => [param, desc[param].default])) return class extends GenericP5Visualizer { // rest is the same...

9. And finally, the only real concern I have about the visualizer-developer level code I suggested is that the `checkParameters()` method has to be declared to take a parameter of type `ParamValues<typeof exampleDesc>`. That seems a bit verbose/redundant/unnatural for  the casual visualizer writer. Unfortunately, it's not valid to leave this parameter untyped, because derived classes are allowed to define overrides with different parameter lists, so there's no way for the compiler to tell that's not what you mean to do. Ideally, there would be some way to provide a single type name like `Parameters`  or `MyChaos.Parameters` that would in context there have the correct type for that argument, but I couldn't figure out a way to do that; perhaps one of you can. If not, the only very slight variant that I could think of was to use a type alias, but I am not sure it is really any better:

const chaosParamSpec = { foo: { type: ParamType.COLOR // etc. etc. } } as const type ChaosParameters = ParamValues

class Chaos extends P5Visualizer(chaosParamSpec) { // other stuff checkParameters(trial: ChaosParameters) { if (trial.foo.blue() > 192) return validationError('Too gloomy!') // etc. }

Here is a [revised playground](https://www.typescriptlang.org/play/?target=99&jsx=0#code/PTAEAUBsEME8HMBOB7ArgOwCagLbIMYDWqADqMgGYTSLQ7QBGkApqALQB8EArAGoCWAZ1TRI-AF7NE7LoJLN8-Cv3ygAbkJFjJiAFAhQAVUH908UA2TIW0dKFOgSMfK0qheo-pmgAXfsnQAZR9fVEFQHwALFFR4SLQfUApkaRMcJxV+H1hdXWZ0VBxqWhwAFVh5UABvADlDAFkAIQBRACUAGlAASRrS5oBxNs7A0tae-oBfUAMAKzDEwWYWfD8zUGgk5gB3JJSIyNZmAA86J2ZdbMrwGjpy+UMsKUhYU3MAXmrpsBrmZmwfZCgOQKJSwfasDCYJ4vNZ3ZiBfCIfgkRKXVjJaTMaD4SLFW4Vc6gImgADa1xKcIAdHUmm0ALoALlABRwDCkumJpPJ+PklJ6fUGrUZzMKbL0nLJNzKBMpIzGNX6wsEPiRZl0E1yph8Ugo2NY3Ol8gAYhgVv50IIADylLhVDnEtSebzagAUtC2TOVqvgAEomZZrFj0PaiYgsdpmG7oB6gSrXn7QKUQ6AoWHPJIXfgrIQ-kzSgmva91bkDIE6KxnRtGAlwXjDcxwtBwrA0KBItA1MxOgFnszkDs8GGgfJFMp8KJe2jMLosxbUQSTegzQFBEyqiSANL2OwGuHC3cL01+FeWg-3R6IZ6vTd0jhTD52iVn5jUhotIWgNfJzmOsTO5hMu6oBvFwACEoHupS9A+DiLrAAAeiSAAMbAAJyUnSABUAAkwA+p0Bh7Fs7bap2iCdC2qCIOEQgWMwPjauKnLEmmEaAdGwFcCQNCLEakDIL4UZbPh37Eqm4YSABoBZsgObYCB0nZn8lIAsE3ouj6yYTO0yaShSMr8gM9KftUolEr+Xi+FJQEKeBkHQbBCEADqYDheEEWAREkcwZFmaArGSexOwKdx1HMF06A+EJInMWJUgSZITIyXJnGKbJymqXGZgaVpOlPlKVJyuMdImY+sXqE6VlBalKqoF2fkBYl-kcQp7p5eV4nplJyV-KlPXTpyGoavoYClAcw4gmOvjmuQVABKwoXlox3ZUBsoV+PgqAwNIz6gBuuhajqep1hFjG6i4lpbsc2pYOEz62smUK6ltPhMs+DypleZg3smaJMvtnIGFs-CQJAzK-P8gJQqIoDA1EoAAEQUCgOAI7s0hRIcLLSU2hJiUIThwDU5aell8DJgYlJU+QmPSCQKDyIgfgNqA8CAgcYbJhZ-4APyZkpmBvQVBIfdC14bnSCYBjYwbDQYACCdhNiY8DoDg+SJG4OawOEGKOFK9FSOEAJ1nCq4XASpsEgAwvEKgsw+JLa6T3r7sL8gagrE2jio00BLN6xJFtYNZqDCjHnYbiLerjHm2idYACINoiyIR9a1ugNd+SYHd7vMLbyD24IXCO1uDja24pTW27JSnUdF1Vzed4jYm43CIz8eziYyqvAHWbpCw2q9ugARsMCPuqNHhvSFCggpyi5px5biuwAaSdz0iC-+x8a-J5vafPgXRccC3hh+GI2QRIC8D0bWU+MREls4oXLi6yjgfj0oKiJ3vqfmgyFtKjNCOCqbEPgDRH1fqeBOmcQHZ3CCvXeG8-4BBLtUTc25QAVyoOABO+4E43hJAAcjREQukw1DqIHOvqKUjAWB1yocdKoXxQBzGVECMQcQfC9jSBkZQfwADc9gfBEPCAwJsvtQZgkxojVi+xXgI2TOgEmsZvTJiEB4P8-orAyy5pVV0UsdFBmTMrfgqsDTT0EBpJkahC4DRYswZGDZIgGisQmWxXhiwtxHgsKirBiJgjYGmTAPYwS0XQBDXqDgZERJARELYUNHGmCyIvToDBUCJH7iQEGUhtzdx8IIKmlJAE0JKJouqVpcGwJujnUAiCpTr3nhHEuFMwDKGookMioJawGgYdQxsQ5FiQAoGwLuQgbqvWTFUrOt0f7IK3ugS0wDQErAgXbKBuCOBcD8jzT4QSsQhPQL2DB5dmAthwXgoW+lzyfRhPAEkyzaCrKlJAhs0COA3hKhqcqJkIlkRYV0KgmMwQ0HyCQzoURaIAAM-lSChbDEGYN6A5ggDA3UIN1iNn1iUaeKZf4LNyPHVo0YDTlLedMuBsz6klEafvc0aCypEn2dAQ5xyy52GwaipUZMvEGAYZAVAUImSK1gacFg2Klq5Nnk080M4VyJGOGK5gjTgKmU5FmRAETqJrietAF6TJuAQoJFcnkL5DKCk6JgAmMBYDE3VkyIh1sUhasEEQ7SyYYBskgDqxxerICvVAI69syBXVGvkCa+sspRjjEtdaomKiiEABlGBLDde1IkSAOxZFgF+H5ur9WgCQpSQ1LDba2HgL3fN-qr6gChdAeFyKWaKoHsq5OSRoAYuIvkUAYRepev7H5P6VteQ0nfJ0PyVq5A2rtVJIh-RaAaGyEQ8dPzSz8BwCDGgzxOhKFAJRaS7YzCsBhaKOFNaoWFjMPC6J41YmJDEBEzozaziNL8kDRFfZEgJMQIQTBpLRAVPINIG9rB8AwEELrMBKR+As37VsPy3MqqgH5ulQWIpWRSB9H1AWoAuBIS0uqTFik5wsIia-QQNBpGAmEBQMcMHIrDxoCgHYMj45uGfSwFVJZRqIDBDictvdbBgjcDIuDNaZFbBQGsNEghOjA1BqzfIUgrKZ0QCgPQXdEjJGQBGslVo0TsZOC2xpDLkwapdUyAA7Om0AnqlgOqcZgZdyZM2LpzYWykAAOXlo0lhgzhAifeuAUisGrBk2GJFQAAHkGAzHDpSZGyAcDNEikiFmISGwt0CL8NsDESCrhAMqbEhBkBkQoPxLYlJ+7AAAI4VIjoIYAAA2SzAAmJrnnWueaa8AMMXoO3oDYKYCgUgwyYDYGiNglApuxfDmwRL6tUswcEAdSK9dWAxbiysJ1c5ECoBWHsMqC2UtxjeaUap8D-IHNCVyBmUhsgbjOZ0ITt4XTJg1ml1c3RGJ0OYNaE+RIEzMJOXYUoyE6TCjBwARi+QInzoAAASSxGZByXA19gbBMe6AoEeGaVafDEq2NAi7VL0Crwafi5pLppVvQTgmQn-6BXkoTraUAZn5USpwF9l0nLcE+hJCVD4m34vaysdKzS6qOfuiZAzqUen3mquFysBLKMTufZdNHQp9ASAa6lKlEk0dOhELIT6CXxI9b8xI9HAOmusOMuYppx+xpTThA+GiRcy4LQkmlQbqUdIVIEhKk2YdzAPcNctEJ7ZPz3S+5KILp3oeXeUk6hGanydY90H9-js3RJvkOJ8FROw7p4djVojiBQhBjaRZu9AEg9NkD034CpzX6whxsl7r2zAaTws9wU+2cI81EZNjRlEXwONlRV-OAYa3v46oJ8bOEWe-AxvIdEFENAcRwQgsgMEkFYY0nSPGmxoFh40eL00jjs--tkrE5mbU6ldBaUoPQBwNPc9aedGl6AWXZSAPM44Fhrpn-pUizmqsSI7prkyDzmcm4HzgLorrNsrqLm-vgDnsRuwv1DLgoCkJgJaLzgnM9mTmgkrj4CrklmrstrriUFrrXhrvriQJ0JCEkhEpgJLGgRbhAXrlHFKIIFhv1BnjgPHu7rjiuN7untHP7miP7o1JGDHhIWgWGAXpqmlClMHozhUu8vDo8mAs1DsPfEbPsGPqCo4H1hrJgv1OjDgAApfp7tUk8gTtGLfpSvfmTkgjKqgigR-kkCjFgRqrgeAHdkzLAI9rAIQbAAAWzkSDLiSnLsAQrvbpwdQVAfgfzvHiQZSMgeLuziRl-j-nQPLpsggVtqQcdktg2FQXQDQTrtbgpAIUbibuwXsJbuwtbtwdQXbn5I7sIUuK7gnmHovGIXPAIZIYHkRs+P0SeJHpEeVHIX7qqt0fgIUinpJC6AtsMWgXnqGPRIXrocWAYJjmwJnFgAHAcJACjjYQ1txq3LRGGITK-NJOBrnCUL9kIv3jWmyIplqt-GyOOL2rDBWAECQlgiPPBkDJFjIvodRAigph8V2nYFkPYOEJ3sUmBsrKAP0EpkiPgAaL9k4TUggq4RTvMs0vYC2otgUnWL9n0kwkouWKqkQhgIQCCegEQuooIJol4KqrqJAIsMmI3o6NqECB2H8I0rTsmO6K4tEVsOof-tkV6PtgCIgJ4aih0T8pCoUhRp2JgCqh8NKoOpEEIJSBKTwaqvjoTigRsXon+FZBpGAbFIoTseqVVgcEQBYrHChoQC6E6ZqSKcnBCgaYUsae0ZaYDGAAAOpoCQDYAfFsgMS5ImyNq1jJCgz9iCYMD9YrA7pAqRbjh2AfH0BQg1rfqEA2YGCmDKgHIBy2CklnDkl+x2Cj5fofpRDMb2BUAInhLICaxkRIiYBQjBjqoumEBulGwem5h1gFEs527TGxS7pjmYBYYtn9iBpdBklmHqxEIznMQOnKG1R4y54mIQZmLoAjnUS2n26cjpGmKqxekBmdDJS3mGk+nal+nyKBkxHBkEachhhOKCAuI8Hnn6mGlBmVGqrXT2HmnenCkvlzz+lCCWlyxgDyzqBSBgiYCijbiMTqxWoqaokQZ0Q+BbC-B2DRy-brDHEbAaDCBdTkSsLzA1p-nLkIm5ktwfGHIvjIb1BgjtLsK+DajpCJBWrYA+LrApk7BCaYVSDYVN6Cl4WLCFKaRyXomYkqDgB8CaA0V4mXYP44BP4LJcB37hAYlfHYm0JMB-aFH26LA1ZMgshigC5ym1QHZKk064KdC-hfZ2VSAC6qnlTtxSAWlAUanMA1aqoeVfkOgwZbAugRJbAeW2WnqIA+V2nMTekhWqqxXhWDRs6IWgA1DLlEWKaoiAisYGlrCFW5nrCbQThhJzi2B+BVQlI8DUkXQUr4l1KEk0qU70qqrGVSCqXqXUURiIAK4ABkE5cRmyLcXQIiVeZeAQcwV+dgcMuIMpIBXAjZSQpgNVEQfm4QAWTShhiQGwalAgQ1kkegBg4iiw2A-sGwFanYO4RJ7hSsKwWgvYbxEJgRzM4QYgFZ2AIGcyL1xSBgAAmq2Bulwpir+gEv8SmF4CyYkImTIrOItbYStRNUzutbDHABYMVusD4C3H1ViTieZe8pSK3KFhaERcBlXmPl0BEGlv8CRN3okAiQkojYpOkDkhjrWKNnsEIC3JtQdUFjXmJVikpSbOurWWYRsPHBjQjEVhtDtUyf2CRd9ctgjCDWAAjv2D5FIJ0CCUdXDcYbvjWomWgNIEjNtZAGjNdaBo8f6RWP1VqQ8crCzJQC3MRCoLiPJmDB8bOIiIbOoBpcNbJnDZVYoIgJtDgI9aiDmfKrQFqBYGCGEIJg8frRjFiYQC3AtqAMEMVlFmVhVomASIFqnD2mKGwPAFRKgKwuOJAOILzciunUpedIqdxSIegJ0IsFPmAJEHlgVsAEVkQKVjqBVlVklrVvVovM1p5p5pZpZk1q1q1sAPEFsBNsgGwFCKiWGBNgSMSaMo8fNjBlGWwAwLAGwGnWYENo8EcGPMeaEJzBcTNKdaHRdVpaTuTl1cSfSsqXzilY7vjiQUUfFqUadlYukZkcnD6FBLQX5DUVwHUXikMRIcnr6i9GweKdscoUpYZcpSZW-edToPEZ0YnQqSkC6NZQlRhklZLClT8v5S5a+dZWgT8leUeTeeqZahg-6iQWw8SJsaAFMMHrFVQyFTQ-ZfQwpGpS1RZSzvDtcHGcoXrIHYoawFRVoBdStvHGWiGqeb0WtZaAZlQBxq2nPCfEpVxXo+EPg0Q1ozoC6GY40r5USMourPSaUHtr2HoxtKyYORXgY-OUyDYwY648xGWVQPOZSHZmDJaKAK1nwSuIGDE8gPAC6EQg8P3OSb3OqSYcgOhfcfHPzdIMHkvmNkQgI1sUoXYP1JPZqgYVwK1qAKNeNXU7EzE-kPAPDFwFDi020wLJSK5tmqAPE1DpSPhqut8F2fbKXfIOXSiMCf2I2Apl9Q3vdstrZuMlEg2eNM422sYfTA2BrG+mAA4BsNY8GgPogcdccarVsJPsvvPp+CGcSArMcRVdWWPWpl4Bo1FbabYNgNiORrWBeiFfCvXh3Nxe-DIpozRbZvrZABClYAAj+P85laIJ5YlclRecSEw5SBoNsDFdsB5VU+gSk-xOk0Qq0A2CEEzN4Ulsum+ZSNZeDphE6cM9kAhVcflTsIPKIlVQXjtXtpHFRMFkQKQKBHKiRtADYxldsKAJc9ACGi6CSFDu0AAMztDcCSxXHZasCbUdk0ToApJdTYAaphgrDPAAJdwUtpMZMF1MwL5MuytXOaQtz1AEC-qmAkAZIAKuvKvvnSk8H1Mur0lNaskBshpGkfmVExMppgwfBEKBCIqwCRs2MxvBvUFDMLojNJtFqeasm2ssCpNUvywKaMVbTRkbYbgusZuIYGKaRRuFLXknkGzumKXJMluUsZOKzCUFWsDtidh1tutXGetEDkCMz1n+v1v-MkitbtAAAs7QTWurHrXreNAN6AvrkyzbmbriobBhSbUOm5BgGg1gVk4Q5mjTCT6BoCWo0rggdrVLvLfYrMVgTmz29b+ikYpuY7G7bMuT40XZHMONoRRGu6cLw1klmqO1-GR62AkJ5se7sT9J+VCwgI4iTmuge7P5fW-51BOUxbL4PbRCr7jFwMZgI7gb+7PBmkQAA) with all of these possible amendments. You should still feel free to adjust/adapt/discuss/etc.

[gwhitney]

Great progress! There are many fewer open conversations (make sure to unhide all comments to be seeing them all). The ones that remain mostly fall into a few categories:

(a) Resolving conflicts with the current ui2 branch below this PR (b) Strict typing of the Paramable interface (c) Handling of empty/default input boxes for parameters (d) Providing at least one example of each of individual-parameter validation and coordinated validation (we are happy to push a commit for this if you like)

although there are just a few miscellaneous ones remaining. When the PR is unconflicted and ready for further review from your perspective, please mark it as such. Thanks so much and looking forward to it.

[maxd0328]

So I've completed making the parameter system type-safe, as well as opting for option C that you proposed for optional parameters. Having said that, the required property is relevant once again, and the parameter system will use the default value if nothing is inputted. Let me know if you have any other questions or concerns. On the other hand, the merge conflicts are not yet resolved. It's related to something that I didn't touch myself, so I want to have my team take a look.

[gwhitney]

So I've completed making the parameter system type-safe

Thank you so much! We really appreciate the effort that you have put into this. In particular, I really like the very understandable names you've given to various of the helper types and functions.

However, the situation as checked in is not yet quite DRY (Don't Repeat Yourself): In OEIS.ts, one now has to specify the paramDesc, which lists oeisId, givenName, cacheBlock, and modulus and gives information from which the realized types of each can be deduced. But then in the definition of the OEIS class itself, there's a sort of boilerplate block:

    cacheBlock = paramDesc.cacheBlock.default as number
    oeisId = paramDesc.oeisId.default as string
    givenName = paramDesc.oeisId.default as string
    modulus = paramDesc.modulus.default as bigint

which reiterates this information, including the types again via as clauses. That will create a maintenance issue: someone working on the code might reasonably think they can just change the paramDesc, but actually they need to make an exactly corresponding change down in this section, too. The goal is to avoid such mandatory concerted future code changes by having just a single location of truth for each piece of information.

This situation can be remedied in this case by changing the definition of Cached from a generic class to a class factory function that takes paramDesc as an argument (not the type of paramDesc as a type parameter) and returns a constructor of a type that has been inferred from the exact type of paramDesc. This is modeled by the P5Visualizer class factory function in this playground that I linked to earlier. Then the entire boilerplate block of property definition lines can simply be deleted.

Unless you tried that technique and ran into some problem? If so, please share the difficulty you encountered, and perhaps we can work out a way around it. Happy to do this by video chat if it would be more productive. In any case, we want to reach a situation in which the list of parameter names and their types is not WET (Write Everything Twice). Thanks for understanding!

[maxd0328]

So as of my most recent commit, the realized (top-level) parameter values should now be generated and known to TypeScript automatically via the class factory function. As we discussed, this unfortunately has to be done for every 'category' of sequence / visualizer, but for now I've done this with Cached and P5Visualizer. The only persisting issue is that these properties are read-only according to the compiler if they're not declared explicitly, but this isn't really an issue with the present visualizers.

Additionally, any remaining comments and merge conflicts have been addressed. I think at this stage the PR is ready for a final review, let me know if there's anything else I should be aware of. Thanks so much!

[gwhitney]

OK, re-reviewing the PR now. Thanks for all of the effort on it.

[gwhitney]

When you change ModFill to Turtle in the const visualizer = line of Scope.vue, it comes up initially with parameter errors: Sequence Domain, showing value "0, 1, 2, 3, 4", complains "Input must be a comma-separated list of integers," as does "Angles." And Start, showing value "0, 0", complains "Input must be two comma-separated numbers." Perhaps there is some bug in the string validation functions for these parameter types, or a bug in the conversion function to their realized types?

[gwhitney]

@katestange @Vectornaut This PR as it currently stands breaks the Grid visualizer, as in nothing appears between the header and the footer of the page, and there is at least one error in the Console. By our usual standards, that would be a blocking deficiency of the PR. Most likely, the difficulty is a conflict between the new strict-typing API for Paramable and the fact that Grid extends its parameter set as you add additional properties to be displayed in the cells of the grid. Quite probably the part of assignParameters() where a team member commented "I wasn't able to figure out exactly what the intent is" and commented out a section of code is indicative of such difficulties. It seems likely that the tension concerning the design of Paramable and the implementation of this visualizer will require a noticeable amount of thought and implementation/testing time to resolve. So, given the relatively short amount of time left in the Delft team's semester, do we want to: (a) Adhere to our usual standards and not pass this PR until all visualizers are operating? (b) merge this PR into ui2, but hold ui2 out of main until they or we can address the Grid issue? (c) merge this PR into ui2 and leave that on track into main, and once it hits, file an immediate high-priority issue to fix the Grid visualizer? (d) something else I am not thinking of?

Kate & Aaron, thanks for weighing in as soon as you are able.

[Vectornaut]

This PR as it currently stands breaks the Grid visualizer

I think we should merge this PR into ui2, with the Grid visualizer moved into the workbench to remind us to keep it out of production. Then I think we should open a high-priority issue to fix Grid, and try to at least patch Grid (as described below) before putting the new UI into production.

Here's why I think we should do this:

• The parameter typing system in this PR seems to work well for most visualizers.
• Adaptive parameters seem like a nice-to-have rather than a core requirement of the parameter typing system.
• As far as I know, we only have one example of adaptive parameters right now, and I'm hesitant to redesign the parameter typing system around a single example, unless that example is crucially important.
• I think we can get Grid back in service pretty soon—maybe even before we put the new UI into production. Since Grid caps the number of properties at twelve, I suspect we could patch it by putting twelve fixed property slots in the settings panel.

[gwhitney]

OK, given Aaron's sentiments, we will not consider a working Grid visualizer as a pre-requisite for this PR to be merged to ui2. Instead, please:

• [x] Move Grid.ts into the visualizers-workbench directory, with a comment that it is not currently working, quite possibly because of incompatibilities with the current Paramable system.

Given that you need to do that, here is a checklist of the unresolved conversations above, which you should also address for this PR to be ready to merge:

• [x] Restore .env (it has only whitespace changes) [Still pending]
• [ ] Handle or create a stub for handling the difference between OEIS sequences (that create a new option each time a new such sequence is made) and other sequences (that are just freshly constructed from parameters each time). [Held over for future PR]
• [x] Either turn the checkParameters in Differences visualizer into individual-parameter validates yourself, or ask us to push a commit doing so
• [x] Fix the comment just above the export of the visualizer module from P5VisualizerTemplate
• [x] Remove the extraneous evaluation of params.formula in sequences/Formula.ts [still pending; if the evaluation is actually needed, please comment as to why]
• [x] Remove the only remaining occurrence of the identifier oeisSeq, in sequences/OEIS.ts
• [x] Fix the parsing of comma-separated lists of numbers, as seen in the current errors in visualizer/Turtle.ts [The situation may have improved somewhat, but the Turtle visualizer is still unable to parse the start parameter, no matter what we enter -- no whitespace, with whitespace, etc.]
• [x] Remove the remaining redundant as-type coercions
• [x] Remove always-true conditions based on the new type constraints [The situation here is improved, but you are still checking for example that the cacheBlock parameter of OEIS.ts is an integer, which seems redundant with the fact that it has type ParamType.INTEGER.]
• [ ] NEW ITEM: Clicking back on the Numberscope logo does not restart the visualizer. [Held over for future PR.]

Thanks so much! We are really close now.

[katestange]

This PR as it currently stands breaks the Grid visualizer

I think we should merge this PR into ui2, with the Grid visualizer moved into the workbench to remind us to keep it out of production. Then I think we should open a high-priority issue to fix Grid, and try to at least patch Grid (as described below) before putting the new UI into production.

I just wanted to chime in and say this seems reasonable to me.

[gwhitney]

@kaldis-berzins or @maxd0328 please confirm that the OEIS sequences are not working in this PR, that you are aware of the situation, and that it will be corrected in future PRs for this project. If OEIS sequences should be working, please explain how I can test them. Thanks.

[gwhitney]

OK, besides the OEIS.ts situation mentioned in my previous post, there are just three checkboxes in the checklist that are not fully resolved (whitespace-only changes in .env; parsing the p5 Vector type; and some remaining redundancies between parameter type values and code checks).

Please let us know if there's any way we can assist with these final cleanups that won't be disruptive to your work -- or if you prefer simply to address them yourselves.

However we also noticed that if you do npm run dev, and go to the localhost link, the chaos visualizer appears. But if you click on the "Gallery" item in the top bar, it goes to the mock Gallery, and from there if you click on the "Numberscope" logo at the top left, it appears to go back to the visualizer except no visualization actually appears (the main area of the screen remains all white). If at this point you do a "reload" in the browser then the Chaos visualization shows up, but you shouldn't have to reload. If you can verify that this behavior will be fixed in a later PR in the pipeline, we won't worry about it in this review. But if not, please fix that as well. Thanks!

[gwhitney]

I guess I didn't explicitly say that I updated the checklist to correspond to the current state of affairs, so that you can just refer to that comment to see what remains.

[maxd0328]

Alright, the final changes you requested should now be there, and to address each point individually:

• Yes, this PR is intended to break OEIS sequences. This is because OEIS.ts is no longer a selectable sequence in itself and the sequence switcher where OEIS sequences can be selected doesn't exist in this PR. We are aware of this and a proper OEIS implementation is coming in a later PR.
• We are indeed aware that clicking on the Numberscope icon doesn't reload the visualizer in this PR. This functionality is part of sequence/visualizer switching/reloading, and is implemented in a later PR.
• About your point where the visualizer doesn't re-render when returning from the gallery, this also concerns a later PR where proper specimen reloading is implemented along with switching.

As for everything else, it should now be fixed in this PR. Thanks!

[gwhitney]

Although the Turtle visualizer is now accepting input and running to produce an image, there is still a bug in parameter processing. If you start it and then change any of the angles (the second parameter in the visualizer) then you get the same image regardless of what you actually enter as the parameter values.

The difficulty seems to be in shared/ParamType.ts -- you can't use for (elt in Array) ... to iterate over an array in JavaScript; you have to use for (elt of Array) ...

Please fix these parameter-parsing difficulties, and remove the extraneous evaluation of params.formula in seqences/Formula.ts (in the checkParameters function), and then test everything (as extensively as you can) to make sure it all works well so that hopefully we will not have to go back and forth again.

[maxd0328]

Apologies for that oversight, in the interest of time and getting all other PRs submitted I hadn't spent enough time doing final bug tests on this PR. I have tried to do some more extensive testing, but it still may not be exhaustive. In any case, the bugs you pointed out and any other bugs I found have been fixed.