search

numberscope / frontscope

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

refactor: Improve uniformity of VisualizerInterface and P5Visualizer #310

Closed gwhitney closed 7 months ago

gwhitney commented 7 months ago

Three related interface changes for the Visualizer hierarchy, as discussed in #293. These are all designed to make some of the details of defining a Visualizer less confusing and more concise.

(A) In P5Visualizer, draw() should be abstract and hence required on runnable derived Visualizers, since any (additional) boilerplate needed by all P5Visualizers would go in the function that wraps this method in the default inhabit() method. Thus implementation draw() functions no longer need to call super.draw(). (B) Since setup() is the default p5 initialization method and can only be called once per sketch anyway, clarify that generally initialization should be carried out in setup() rather than inhabit(), in derived Visualizers. (C) Since name is used as an instance property in SequenceInterface indicating the details of the particular interface, re-designate that information as the return value of a visualization() method of an entity implementing the VisualizationInterface, and suggest that it should only depend on the class of the entity, not the instance. Implement this in the P5Visualizer hierarchy with the visualizationName static property, and modify the VisualizerExportModule constructor to look for it there, so that the visualizationName will not have to be stated twice in every module implementing a Visualizer. At such time as there are other base classes for concrete Visualizers, these conventions should probably be moved one level up to a common base class of all visualizers; there is nothing specifically related to p5 about this naming scheme.

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.

gwhitney commented 7 months ago

(A) Oops, I accidentally pushed to origin rather than my fork when creating this PR. I will try to be more careful.

(B) There were likely some doc-page changes that should go with this, but my belief is that Aaron will merge or rebase this into #293 where there are overlapping doc changes and straighten it all out. @Vectornaut if that's not right, let me know, and I can add a doc commit to this PR as well.

gwhitney commented 7 months ago

OK, all visualizers have a description. Should we enforce this going forward, or leave it optional as it has been to date?

katestange commented 7 months ago

OK, all visualizers have a description. Should we enforce this going forward, or leave it optional as it has been to date?

I don't know... Yes, I'm imagining this might be a tooltip text or populate some type of menu or explorer or some type. I leave it to you to decide if it's useful to make mandatory here. It could be something that depends on the UI revamp.

gwhitney commented 7 months ago

OK, I will remove any periods from descriptions that have them since I think they are mostly not full sentences. And I will make descriptions mandatory, since we now have them all and they seem like they might be handy in a UI revamp, if we ever have a chance to do one ;-)

gwhitney commented 7 months ago

Done. If all looks good to you, Kate, let's let @Vectornaut merge this one so he can time it most smoothly with his work on #293.

katestange commented 7 months ago

looks good!

gwhitney commented 7 months ago

OK, Aaron, ready for you to push the merge button whenever it works for you.

Vectornaut commented 7 months ago

Implement this in the P5Visualizer hierarchy with the visualizationName static property, and modify the VisualizerExportModule constructor to look for it there, so that the visualizationName will not have to be stated twice in every module implementing a Visualizer.

I like this simplification!