refactor: Improve uniformity of VisualizerInterface and P5Visualizer #310
Conversation
|
(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. |
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.
|
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. |
|
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 ;-) |
|
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. |
|
looks good! |
|
OK, Aaron, ready for you to push the merge button whenever it works for you. |
I like this simplification! |
| <h5 class="card-title"> | ||
| {{ `${viz.visualization()} of ${seq.name}` }} | ||
| </h5> | ||
| <p class="card-text"> | ||
| This is a bundle of {{ seq.name + ' + ' + viz.name }}. | ||
| Display {{ seq.name }} using a {{ viz.visualization() }} | ||
| visualization. |
There was a problem hiding this comment.
These new messages are much clearer to me.
- Discuss the new `inhabit()` / `depart()` scheme - Explain that `super.setup()` calls `createCanvas()` - Note that a `draw()` implementation is required (as of numberscope#310) - Remove reference to `super.draw()`, which is removed in numberscope#310
Incorporate numberscope#310.
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
nameis used as an instance property in SequenceInterfaceindicating the details of the particular interface, re-designate
that information as the return value of a
visualization()methodof 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.