Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add base and multi controllers #255
Add base and multi controllers #255
Changes from all commits
9724363
22ea55d
6e18351
ead72b3
2de8ec1
1c9bacd
bfdec7e
64b712f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand why GenericController contains non-widget components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes it flexible for when you have multiple widget components that can mutually affect other properties - example from the original PR (not updated to this GenericController standard) is the RotationController: https://github.com/catalystneuro/nwbwidgets/blob/add_two_photon_color/nwbwidgets/controllers/image_controllers.py#L7-L29
Two buttons that affect a common value that is then used to determine some aspect of the final visualization.
Also, some attempts to use properties of widgets (an example being the fields of a DropDown) do not update in-place, so having a secondary attribute that is modifiable can be handy as well if you ever want interactions with those properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, but why throw all of these into one dictionary? I don't really understand the logic of this PR in several ways so let's talk about it when we meet tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For input, they naturally form key/value pairs as attributes. As far as why
components
itself is also saved as an attribute (which might seem redundant), yeah that's largely there for theMultiController
which has the added behavior of more human-readable nesting of sub-controller components if someone ever wanted an easier direct reference to it as opposed to navigating lists of nestedchildren
. That is,some_multi_controller.components["SomeSubController"]
instead of trying to findSomeSubController
out of potentially several indices ofsome_multi_controller.children
, which could get even worse when you start makingMultiControllers
out ofMultiControllers
.I'd be OK with removing
components
as a stored attribute for theBaseController
and theGenericController
and only have it be present for theMultiController
.I'd say it's better to ask as many questions as you can in text form so we have a permanent reference to look back on and also bring in @h-mayorquin and @luiztauffer who contributed a lot to early thoughts on designs for this kind of stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand why you would want to gather all of the widgets into a single dictionary. I think that's a good idea. But why also put state attributes in there instead of just making them standard attributes of the class instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. I can adjust it so that
.components
only storesipywidgets
object references. Other attributes can then be retrieved directly or viaget_fields()
(they convenience retrieval for seeing what attributes the class has instead of looking at it's magic__dict__
)