feat: port histogram model #55
Conversation
Wraps the VispyHistogramView up with some Qt widgets for control. Derived from the histogram example
There was a problem hiding this comment.
hey @gselzer, good to see some of this coming together!
I've listed a few thoughts. I don't all that convicted one way or the other about anything, but there are just some vague "smells" at this point that I need to keep thinking about. It all feels very abstracted and verbose. I know it very much models the patterns I'm playing with in the mvc branch, but I'm still not quite sure it needs that much.
One thing that's worth noting: if the histogram and all of its interactive elements like clims handles and stuff are purely vispy/pygfx objects, then we don't actually need Qt and Jupyter wrappers at all. That is already provided for us by vispy & jupyter_rfb directly. We only need these qt/jupyter views when we're adding additional sliders and buttons that are also controlling the model
| NumpyNdarray = Annotated[np.ndarray, _NumpyNdarrayPydanticAnnotation] | ||
|
|
||
|
|
||
| class StatsModel(NDVModel): |
There was a problem hiding this comment.
one immediate thought I'm having is that i'm not sure this object needs to be a "model" at all. It doesn't really feel like a part of the "state" that we need to track (for example, something that we would ever need to serialize). It seems like it will almost always be derived from other state (specifically, the data_wrapper and the current_index): i.e. when the sliders change or some other user interaction happens that requires us to recalculate a histogram, some object will definitely need to receive that new data and calculate a histogram, but whether it needs to have the full model/view/controller treatment is something i'm not yet convinced of. So, it's possible we could reduce the complexity here a bit by not treating this as a first class object that we pass in to __init__ functions and stuff, but rather just as an implementation detail.
| def setName(self, name: str) -> None: | ||
| # TODO: maybe show text somewhere | ||
| self._backend.setName(name) | ||
| self._backend.refresh() | ||
| pass | ||
|
|
||
| def setLutVisible(self, visible: bool) -> None: | ||
| self._backend.setLutVisible(visible) | ||
| self._backend.refresh() | ||
|
|
||
| def setColormap(self, lut: cmap.Colormap) -> None: | ||
| # TODO: Maybe some controls would be nice here? | ||
| self._backend.setColormap(lut) | ||
| self._backend.refresh() | ||
|
|
||
| def setGamma(self, gamma: float) -> None: | ||
| self._backend.setGamma(gamma) | ||
| self._backend.refresh() | ||
|
|
||
| def setClims(self, clims: tuple[float, float] | None) -> None: | ||
| self._backend.setClims(clims) | ||
| self._backend.refresh() | ||
|
|
||
| def setAutoScale(self, autoscale: bool | tuple[float, float]) -> None: | ||
| self._backend.setAutoScale(autoscale) | ||
| self._backend.refresh() |
There was a problem hiding this comment.
the fact that both jupyter_view and qt_view have all these methods, and all both of them are doing is controlling the same backend object is a good indication that this should be controller logic, not front-end view logic. I suspect you were probably just mirroring how I have get_canvas_class in my QViewerView, but that was a temporary pattern (it has a note on it about moving it to the controller) and has been removed in my current branch.
kinda makes me feel like, in the interest of not wasting your time, i finalized some of those (very experimental) pattern in my branch before you start to extend them for other mvcs? 😬
There was a problem hiding this comment.
this should be controller logic, not front-end view logic.
as an exercise, you should try to use the same controller for both the mvc_histrogram.py and notebook examples. thats' where most of this should go I think...
| class JupyterCursor: | ||
| def __init__(self, native: Any) -> None: | ||
| # FIXME | ||
| self._native = native | ||
|
|
||
| def set(self, type: CursorType) -> None: | ||
| if type is CursorType.DEFAULT: | ||
| self._native.cursor = "default" | ||
| elif type is CursorType.V_ARROW: | ||
| self._native.cursor = "ns-resize" | ||
| elif type is CursorType.H_ARROW: | ||
| self._native.cursor = "ew-resize" | ||
| elif type is CursorType.ALL_ARROW: | ||
| self._native.cursor = "move" |
There was a problem hiding this comment.
i'm not convinced for the need for get_cursor_class and this yet. I do like that you've added a new front-end agnostic CursorType enum concept. but it might be better to just have the front_ends objects to have a set_cursor(self, cursor_type: CursorType) method, rather than this object that maintains a pointer to some native object so that it can mutate it. need to keep looking...
This PR ports the work done in #52 to the
v2-mvcbranch, performing significant restructuring of the business logic. It does not (or shouldn't 😅) integrate with the existing model/view (yet), although the goal is to make it trivial to do so later via modular components.Notably, this PR also currently includes support for Jupyter (no pygfx yet though 😢)