refactor: kill the _ArrayDataDisplayModel, replace with pure functional resolver#233
refactor: kill the _ArrayDataDisplayModel, replace with pure functional resolver#233tlambert03 merged 25 commits intopyapp-kit:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #233 +/- ##
==========================================
+ Coverage 86.21% 86.60% +0.38%
==========================================
Files 46 46
Lines 5224 5196 -28
==========================================
- Hits 4504 4500 -4
+ Misses 720 696 -24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ze connection logic
…ayModel for multichannel modes
|
ok @gselzer... if you want to have a look i think this is about ready. |
gselzer
left a comment
There was a problem hiding this comment.
These are great changes! I think that the ResolvedDisplayState is a definite improvement over _ArrayDataDisplayModel!
I had thought about many of these things elsewhere, too, so good to see!
| self._view.set_visible_axes(new.visible_axes) | ||
| ndim = len(new.visible_axes) | ||
| self._canvas.set_ndim(cast("Literal[2, 3]", ndim)) | ||
| self._clear_canvas() |
There was a problem hiding this comment.
The goal of this line is to git rid of 2D images when you're adding 3D volumes and vice versa, right? Would appreciate a comment so I remember later why you only have to clear when changing visible axes and not with any of these other changes
There was a problem hiding this comment.
yeah, it's because that switch fully changes the textures i think. FWIW, this is mostly just the same logic we used to have in _on_model_visible_axes_changed, which looked like this:
def _on_model_visible_axes_changed(self) -> None:
self._view.set_visible_axes(self._data_model.normed_visible_axes)
self._update_visible_sliders()
self._clear_canvas()
self._canvas.set_ndim(self.display_model.n_visible_axes)
self._request_data()| self, old: ResolvedDisplayState, new: ResolvedDisplayState | ||
| ) -> None: | ||
| """Apply diff between old and new resolved state to the view.""" | ||
| with self._view.currentIndexChanged.blocked(): |
There was a problem hiding this comment.
Am I correct in assuming that the goal here is to only make the minimal set of changes to the view to avoid things like canvas flickering? Would be nice to explain why we have all this complexity in comparing all of these fields in old vs. new state instead of some single synchronization pattern with the new state
There was a problem hiding this comment.
more or less yeah: the basic idea is
- datawrapper + user model -> resolved state
- diff the resolved state to update only the parts of the view that need to change
the old pattern was actually doing all the same stuff ("when this changes or is different, then do this"), but the complexity was spread over a lot of different methods: _on_model_visible_axes_changed, _on_model_channel_axis_changed, _on_model_current_index_changed, _on_model_channel_mode_changed. And, it was actually getting really hard to reason about and keep track of which callbacks were tracking/mutating which state. It felt sort of manually wired up, and each time I wanted to add new things (such as channel names in #225 or xyz scale in #165) i found myself basically having to read through many different methods to figure out what was going to happen in response to a certain change.
So, this consolidates that change -> resolve -> diff -> update view into a single place.
My initial feeling was also that this would be more wasteful or complex, and at first it felt to me like this was sort of defeating the purpose of having specific field events in the first place (and to an extent, it IS). But the key realization was the fact that the "resolved" model is always spread across two different things: the user intention/overrides, and the current data cube. so, one I realized that, (and then also realized that we were basically doing all these computations anyway in the resolution path) this was the pattern i landed on that would simplify the mental model (for me at least).
so, I guess I do see this as some single synchronization pattern with the new state. It's synchronizing the view with the new resolved state. and not wasting GUI updates on things that aren't part of the diff.
related: #196 (which was reverted due to failing tests)
_ArrayDataDisplayModel— the glue layer nobody liked_resolve.pymodule — pure functionresolve(model, wrapper) → ResolvedDisplayStateproduces a frozen snapshot of display state. No model mutation, no side effects, easier about._apply_changes()method handles all view updates. (reduced signal handlers each assembling their own combo of side effects)_re_resolve()- (this can still change if needed... but at the moment we don't need it)GRAYSCALEmode — resolve() already ignores it, so we preserve user intent through mode round-trips.(canvas_widget, viewer_model). Simpler API, lesscoupling. (this is the same nice result that feat: Purge ArrayDataDisplayModel from ArrayViews #196 had)
ndimToggleRequested(bool)— controller picks the z-axis, not the view (again, similar to feat: Purge ArrayDataDisplayModel from ArrayViews #196)