Skip to content
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

Merged
merged 7 commits into from
Mar 28, 2022
Merged

Conversation

gwhitney
Copy link
Collaborator

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 #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.)

  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.)
@gwhitney
Copy link
Collaborator Author

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.

@gwhitney
Copy link
Collaborator Author

gwhitney commented Mar 1, 2022

OK I think that takes care of everything outstanding

@liammulh
Copy link
Member

liammulh commented Mar 7, 2022

@katestange, I'm done with my review. Feel free to merge if/when you are done with yours.

@gwhitney
Copy link
Collaborator Author

gwhitney commented Mar 7, 2022

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:

  • Document in Paramable how parameter typing works and what forceType is for and the special values like 'color' that forceType can have.
  • Add a new 'integer' forceType that stores its value in a JavaScript number but ensures at the UI level that the value is an integer, and use that type for existing parameters where appropriate.
  • Add more explanation of what the visibleValue property of a parameter is and how to use it in comments of Paramable

@gwhitney gwhitney changed the title refactor: Overhaul param system WIP: refactor: Overhaul param system Mar 7, 2022
@gwhitney gwhitney marked this pull request as draft March 7, 2022 22:04
@gwhitney
Copy link
Collaborator Author

gwhitney commented Mar 7, 2022

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.
@gwhitney gwhitney changed the title WIP: refactor: Overhaul param system refactor: Overhaul param system Mar 11, 2022
@gwhitney gwhitney marked this pull request as ready for review March 11, 2022 03:32
@katestange
Copy link
Member

katestange commented Mar 14, 2022

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.

@katestange
Copy link
Member

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.

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?

@katestange
Copy link
Member

katestange commented Mar 14, 2022

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.

@gwhitney
Copy link
Collaborator Author

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.

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?

@gwhitney
Copy link
Collaborator Author

gwhitney commented Mar 15, 2022

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?

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.

@gwhitney
Copy link
Collaborator Author

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).

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.

@gwhitney
Copy link
Collaborator Author

This: #108 (comment) is the comment that still needs a bit more feedback, thanks!

@katestange
Copy link
Member

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?

@gwhitney
Copy link
Collaborator Author

OK I will wait to hear the thoughts of @liam-mulhall.

@gwhitney
Copy link
Collaborator Author

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.

@katestange
Copy link
Member

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

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. :)

@gwhitney
Copy link
Collaborator Author

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.

@katestange
Copy link
Member

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.

@gwhitney
Copy link
Collaborator Author

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.

@gwhitney gwhitney marked this pull request as ready for review March 15, 2022 19:34
@gwhitney
Copy link
Collaborator Author

OK, see how you like this. :)

@katestange
Copy link
Member

I've just been playing with it and I am very happy with it. :)

@liammulh
Copy link
Member

Okay, I also played/tested and I think it can be merged. Thanks, @gwhitney!

@liammulh liammulh merged commit 16bdeb7 into numberscope:main Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants