-
Notifications
You must be signed in to change notification settings - Fork 15
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
refactor: Overhaul param system #108
Conversation
To allow params to be of any type (rather than just `string | number | boolean` and to allow params to correspond to correctly-typed top-level properties, this commit creates a common base class Paramable for Sequences and Visualizers that handles communication of params with the UI. Basically, a class just derives from Paramable, defines a `params` property with objects describing the top-level properties that should be params, and defines a checkParameters() function that determines if a collection of candidate values (as `value` properties in the `params` object) are acceptable. If so, they are copied automatically into the implementing object's top-level properties. If the implementation changes any of these properties and wants the change to be reflected in the UI, it must call `this.refreshProperties()` manually. Also, extends SeqVizParamModal.vue to present reasonable UI interfaces for a much larger variety of param types, as well as adds features such as hidable params, indentation, and so on. Resolves numberscope#55. Resolves numberscope#56. Resolves numberscope#60. Resolves numberscope#62. Resolves numberscope#82. (Although the graphic presentation for collapsible subgroups could be more polished.) Also takes care of numberscope#45 in all sequences and visualizers (there are still a few gratuitous console.logs in components, though). Lays groundwork for numberscope#68, in that the channel for communication between the UI and the sequence/visualizer is now very clear and as close to direct as possible (I don't think we want it any more direct; if it were, then in the midst of typing the visualizer could be left in an invalid state.)
Have verbally responded to all review comments but may or may not be able to supply the commit implementing them for a few days, thanks for understanding. |
OK I think that takes care of everything outstanding |
@katestange, I'm done with my review. Feel free to merge if/when you are done with yours. |
In joint discussion/review at the weekly meeting, Liam and I came up with this task list, so I am marking this as WIP not to be merged until the task list is done:
|
Also the following question came up in the discussion at the meeting, but Liam and I did not settle on a answer: Should the panel showing parameters with their input boxes and explanations indicate the desired type for the parameter? If so, how and where on the panel should it be indicated? Or is that too much information that will just clutter the panel, since mostly the designations are pretty intuitive (like the "modulus" should be an integer, etc.)? Please post with your thoughts on this, thanks. |
This special type for a parameter stores its result in a `number` but enforces in the UI that the value be an integer. This commit also adds this forceType to all Sequence and Visualizer parameters for which it is appropriate. That change noticeably reduces the amount of validation that needs to occur in the TypeScript code.
I've been playing with it and I have a suggestion. I'm using the Chaos visualizer. When I click on "Show additional parameters for dots" I get a new section appearing, but it is indented. When I choose "Index" or "Highlight_one_walker" in "Colour dots by", I also get new options appearing, but the block is not indented. I think this behaviour should be the same: both indented blocks, probably. I would even enjoy a light grey background shading (or similar) to help indicate a chunk of newly appearing parameters. This would make the user aware what they have to pay attention to. I would have the light grey background include the "Colour dots by" or whatever option it is that produces the box, as this would also indicate to the user what is going on (what option made those things appear). Or some variation on that that would also indicate similar info to the user. |
Interesting question. Here's a possible solution: the programmer of the visualizer has the option to specify in the parameter prompt if they feel it is necessary, so we don't need to change anything in the way the box pops up originally. But when something doesn't work, e.g. when the system is deleting your errant "x" in an integer input box, (and only then, to avoid advance cluttering), the type could be indicated by the system in a small font immediately below and flush left with the input field. This would be a sort of "just in time" approach to the issue. Is it feasible? |
Another suggestion I have, but which may not be in the scope of this PR, is that when validation errors occur, it would be nice if the instructions appeared not up at the top, but next to the offending input boxes (before or after). Then at the top, there could be a generic message to "see errors below" and the errors below could be highlighted with a red background highlight. Or something along those lines. (A la standard web form behaviour.) In fact, I'm a bit on the fence about whether to include a message up top at all. I'm thinking ahead to the panel that will hot-modify parameters in the draw view, and there, you don't want the panel to "jump downward" to give an error somewhere up above -- you just want the error to appear next to where the user is playing. But you probably want a top message in the pop-up model we have right now. |
These are nice ideas. Right now, that one block that's indented is indented because I set a parameter on the params, not specifically because they are appearing/disappearing. So your suggestion begs the question: Should I add a "background" parameter, and specify "indent" and "background: lightgrey" on all of the parameters you list? Or, on the other hand, should I actually remove the "indent" parameter and instead make it so that the web CSS makes appearing/disappearing parameters always be indented and light grey background? |
Something along these lines should be doable. I will try to add a commit implementing something like this in the next roughly 1 week. Will mark this as draft until I submit this. |
That does seem like a good idea, but it would require an extension to the ValidationStatus interface -- basically there would need to be a place to communicate the information of what the offending parameter is. So I think it is actually out of scope for this PR -- it would be great if you can file a separate issue that raises this. Also, I think any solution to this new point will have to resolve #58. |
This: #108 (comment) is the comment that still needs a bit more feedback, thanks! |
I'm not really sure... maybe Liam has a suggestion on this point? |
OK I will wait to hear the thoughts of @liam-mulhall. |
OK, let me know how that is on the type reminders for integers. Waiting to remove the draft status until we reach consensus on the api for conditionally visible parameters vs. explicit indentation/background properties and I implement it. |
I think maybe I just don't know what the ramifications of the two choices are, or pros/cons? I'm just not enough of a programmer. :) |
Well, the "define indent and background and just specify them on whichever parameters you want to get the look you want" gives you more layout possibilities, but it imposes less structure. Right now, the only examples we have where we want a different background and indenting are exactly the parameters that are sometimes visible and sometimes not. If we have a hunch that for the medium-term future those are the only parameters that we're going to want to have those characteristics, and that we will always want sometimes-visible parameters to have those characteristics, then it would be better to drop the properties and just do the styling to make those characteristics a consequence of being hide-able. On the other hand, if having indent and background opens up the imagination of various useful/attractive/interesting ways to lay out parameter panels,then I'd say we should go the first route. I'm not really sure, which is why I was looking for input from others involved. Happy to have your thoughts. |
About indent and background. Your description helps a lot, thank you. I don't feel strongly -- you can do what you think is best, as author here. I think the philosophy so far has been to have the parameter UI part of frontscope and not under the control of individual visualizers. And right now, our purpose with it is closely tied to hide-ability. So until we have some bright ideas about future designs, or visualizers ask for more functionality, I guess I would go with the option that ties it to hide-ability? I could certainly be swayed by a description of how the other option will help us in future. |
OK, that's enough for me -- I will eliminate the indent parameter and make indentation and grey background an effect of hide-ability. (That's the way I was leaning but didn't want to unduly influence others' views.) @liam-mulhall if you think the other direction would have been decidedly better I will also be happy to revert to that, so definitely still feel free to chime in. |
OK, see how you like this. :) |
I've just been playing with it and I am very happy with it. :) |
Okay, I also played/tested and I think it can be merged. Thanks, @gwhitney! |
To allow params to be of any type (rather than just
string | number | boolean
and to allow params to correspond to correctly-typed top-level properties,
this commit creates a common base class Paramable for Sequences and
Visualizers that handles communication of params with the UI.
Basically, a class just derives from Paramable, defines a
params
propertywith objects describing the top-level properties that should be params,
and defines a checkParameters() function that determines if a collection
of candidate values (as
value
properties in theparams
object) areacceptable. If so, they are copied automatically into the implementing
object's top-level properties. If the implementation changes any of these
properties and wants the change to be reflected in the UI, it must
call
this.refreshProperties()
manually.Also, extends SeqVizParamModal.vue to present reasonable UI interfaces
for a much larger variety of param types, as well as adds features such as
hidable params, indentation, and so on.
Resolves #55.
Resolves #56.
Resolves #60.
Resolves #62.
Resolves #82.
(Although the graphic presentation for collapsible subgroups could be
more polished.)
Also takes care of #45 in all sequences and visualizers (there are still
a few gratuitous console.logs in components, though).
Lays groundwork for #68, in that the channel for communication between
the UI and the sequence/visualizer is now very clear and as close to
direct as possible (I don't think we want it any more direct; if it were,
then in the midst of typing the visualizer could be left in an invalid
state.)