Better gui for many channels (wx-specific) #195

gnodar01 · 2025-05-13T21:52:56Z

Creates a scrollable area for LUTs
Creates a multi-select to choose which LUTs go in the scrollable area

Resolves #193

- only numbers in display options - only show display options if composite - TODO: only display selection options on mode change

codecov · 2025-05-13T22:01:10Z

Codecov Report

Attention: Patch coverage is 96.85864% with 6 lines in your changes missing coverage. Please review.

Project coverage is 85.73%. Comparing base (758d519) to head (774c7c7).

Files with missing lines	Patch %	Lines
src/ndv/views/_wx/_array_view.py	96.85%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #195      +/-   ##
==========================================
+ Coverage   85.23%   85.73%   +0.50%     
==========================================
  Files          46       46              
  Lines        4963     5146     +183     
==========================================
+ Hits         4230     4412     +182     
- Misses        733      734       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gnodar01 · 2025-05-13T22:07:47Z

@tlambert03 @gselzer Implemented what was requested in #193.

Screen.Recording.2025-05-13.at.5.57.49.PM.mov

Draft because it could use a bit of cleanup, but also there's a bit of a problem around lifecycle control. In the video you can see when I switch back to composite mode, all of the channels are visible instead of only the ones selected in the options.

First channel mode is WxArrayView.set_channel_mode is called and then after each WxLutView.set_visible is called. If this happened in the opposite order I could emit an event after set_channel_mode to call set_visible on only the luts that are selected in the multiselect options (at the cost of having two calls of set_visible per LUT view per mode change).

There's other approaches, but I wanted to avoid a state injection on WxLutView that conditions set_visible on whether its enabled in the multiselect options.

Plenty of other approaches, but wanted to get your thoughts first. TBH the scroll area gets me most of what I want anyway, so I wouldn't bemoan getting rid of the multiselect in general. But if you like it, any advice on how we could change up the lifecycle, or whether that's the wrong way to think about it?

Also haven't looked into putting this into the tests yet, but any pointers would be appreciated.

gselzer · 2025-05-14T15:25:39Z

@gnodar01 this is great! Thanks so much for jumping in!

TBH the scroll area gets me most of what I want anyway, so I wouldn't bemoan getting rid of the multiselect in general.

Could always split this into multiple PRs!

There's other approaches, but I wanted to avoid a state injection on WxLutView that conditions set_visible on whether its enabled in the multiselect options.

Yeah, that's tricky...without knowing too much about wx, do you think the lut selector could tap into the wx.ShowEvent emitted by the LutView's set_visible? It seems like Event filters in wx are pretty coarse-grained, which is unfortunate...

gselzer · 2025-05-14T15:38:01Z

Also haven't looked into putting this into the tests yet, but any pointers would be appreciated.

Oh, and by the way, I think the place to test this behavior would be here

tlambert03 · 2025-05-14T17:48:34Z

just wanted to also express my enthusiasm and gratitude :) will have a look soon if @gselzer hasn't already given it a full review by then

gnodar01 · 2025-05-14T18:31:14Z

Of course! Thanks for saving me a ton of time by creating ndv.

Okay, so I ended up fixing by taking a different approach such that WxLutView.set_visible now emits rather than calling Show/Hide on the widget directly. Then we validate later in WxArrayView and call Show/Hide only if necessary.

When reviewing, please do pay attention to my use of Signal. It's a really great tool (love the observer pattern), but I fear I may have abused it.

Will add tests in the meantime.

tlambert03

just a few high level observations and thoughts, and a couple inline nits

if there are only a few channels, that "channel display options" row takes up some unfortunate space. So I wonder whether the whole behavior (or at least the visibility of that option) should be gated behind some minimum number of channels?
i wonder whether toggling the inclusion of a channel in the channel display options should also toggle the visibility of the channel itself? (that is, do we ever want a channel to be visible on the canvas that doesn't show up in the list of channels)? I noticed that by trying it with a 16 channel image, choosing "select none", and being a little confused at first at what I was looking at, and why it had so many channels
not for this PR, but a note for my future self. the relatively slow performance with 16 channels, even when only one of them is showing at all, makes me wonder whether we're making unnecessary slicing and/or rendering calls for non-visible textures

src/ndv/views/_wx/_array_view.py

gnodar01 · 2025-05-15T20:36:44Z

if there are only a few channels, that "channel display options" row takes up some unfortunate space. So I wonder whether the whole behavior (or at least the visibility of that option) should be gated behind some minimum number of channels?

Good idea, will add that.

Update: Done ✅

i wonder whether toggling the inclusion of a channel in the channel display options should also toggle the visibility of the channel itself? (that is, do we ever want a channel to be visible on the canvas that doesn't show up in the list of channels)? I noticed that by trying it with a 16 channel image, choosing "select none", and being a little confused at first at what I was looking at, and why it had so many channels

Didn't even think about that. Will add that as well.

Update: Done ✅

I have it so it automatically sets visibility off when display is off. When display is set to on, you then have to set visibility on manually. That felt more natural to me, but can easily make it so display on triggers visibility on too.

- reverts emit when setting lut visibility - reintroduces bug where all channels displayed on channel mode change - setup for alternative approach taht doesn't rely on emitting when setting visible

- allows remembering display and visibility setttings across channel mode changes

gnodar01 · 2025-05-19T20:41:59Z

@tlambert03 @gselzer Tests done. Ready for review, and merge if no problems. Happy to address any additional changes as well.

gselzer

This has made some nice steps forward since I last looked! Thanks again @gnodar01!

I have it so it automatically sets visibility off when display is off. When display is set to on, you then have to set visibility on manually. That felt more natural to me, but can easily make it so display on triggers visibility on too.

I actually feel the opposite, although I wouldn't say I feel terribly strongly:

If you click "Select None" and then "Select All", it's unfortunate that to actually see the data you'd then have to go through and click all of the "Visible" checks for each channel.
If your goal is just to visualize Channel 5 (of some large number of channels), you might click "Select None", and then you click on Channel 5 in the "Channel Display Options". But then there's the additional inconvenience of checking the visible box.

(that is, do we ever want a channel to be visible on the canvas that doesn't show up in the list of channels)?

There's a lot of overlap, isn't there? 😅

In fact, I think another question has merit: Do we ever want a channel to show up in the list of channels if it is not visible on the canvas?

Certainly, if you're below the threshold required to show the LUT selector, then you need all of the channels in the list.

However, whenever the LUT selector is present, the two checkboxes provide a bit of redundancy, and a possible source of bugs, no?

I know it's late in the development process here (especially those tests, very nice 😍), so feel free to disregard, but I was inspired to draw a potential alternative design, which would only expose a single source of "LUT visibility" - thoughts @tlambert03, @gnodar01?

gselzer · 2025-05-20T16:51:32Z

src/ndv/views/_wx/_array_view.py

-# mostly copied from _qt.qt_view._QLUTWidget
+class _LutChannelSelector(wx.Panel):
+    # actually set[ChannelKey] | set
+    selectionChanged = Signal(set)


Is this signal actually necessary in practice? I tried commenting out wdg.lut_selector.selectionChanged.connect(lambda: self._wxwidget.Layout()), and I couldn't notice the effect...

gselzer · 2025-05-20T19:52:29Z

src/ndv/views/_wx/_array_view.py

    def set_visible(self, visible: bool) -> None:
-        if visible:
+        if visible and self._display_status != _DisplayStatus.HIDDEN:
            self._wxwidget.Show()
-        else:
+            self.lutUpdated.emit()
+        # elif visible do nothing
+        elif not visible:
            self._wxwidget.Hide()
+            self.lutUpdated.emit()
+
+    def set_display(self, display: bool) -> None:
+        if display and self._display_status == _DisplayStatus.HIDDEN:
+            self._display_enable()
+            self.lutUpdated.emit()
+        # elif display do nothing
+        elif not display:
+            self._display_hidden()
+            self.lutUpdated.emit()


Can you provide some clarification and comments on what the difference between these functions is? Not sure I really understand...

gselzer · 2025-05-20T19:59:33Z

src/ndv/views/_wx/_array_view.py

+
+        self.lut_selector.Hide()
+        self._lut_toolbar_panel.Hide()
+        self._lut_toolbar_shown = False


Could just rely on the state of self._lut_toolbar_panel or self.lut_selector instead of using a separate variable, since their visibility always seems to be synced...

gselzer · 2025-05-20T20:13:55Z

src/ndv/views/_wx/_array_view.py

+        if len(self._luts) < self._wxwidget._toolbar_display_thresh:
+            self._wxwidget.toggle_lut_toolbar(False)


I'm guessing there's a potential bug here where you have self._wxwidget._toolbar_display_thresh channels, some of which are not displayed. If you then remove a lut view to go below the threshold, making the toolbar vanish, you then have no UI controls to visualize the channel?

...I don't think this method is currently called by anything though, so I don't know, maybe just throw in a FIXME?

gselzer · 2025-05-20T20:19:49Z

tests/views/_wx/test_array_view.py

+    # channel_mode = viewer._wxwidget.channel_mode_combo
+    # viewer.set_channel_mode(ChannelMode.GRAYSCALE)
+    # _processEvent(wxapp, wx.EVT_COMBOBOX, channel_mode)
+
+    ## all channels should be hidden
+    # for ch, lut_view in viewer._luts.items():
+    #    if type(ch) is int or (type(ch) is str and ch.isdigit()):
+    #        assert not lut_view._wxwidget.IsShown()


Just wondering about the status of this code. Is this something you wanted to keep or move somewhere? Or does it not pass (yet)?

gselzer · 2025-05-20T20:56:54Z

Aww, man, double review?! Sorry! 🫣

EDIT: Wow, my review appeared on my machine as two reviews, so I hid one and refreshed, and now only the hidden one remains! Sorry for the confusion!

gnodar01 and others added 5 commits May 12, 2025 10:30

Create fixed size scroll area

2d121f2

Add channel selector

2876cfd

Setup signaling for updating layout

316643f

Make display options smarter

2d69800

- only numbers in display options - only show display options if composite - TODO: only display selection options on mode change

style(pre-commit.ci): auto fixes [...]

3d34227

gnodar01 and others added 2 commits May 14, 2025 14:23

Display previously visible items on mode change

732bb2e

style(pre-commit.ci): auto fixes [...]

5ea44a1

gnodar01 added 2 commits May 14, 2025 14:34

Make update_lut_view private

0ac7653

'visible' -> 'displayed' to avoid ambiguity

bcff280

tlambert03 reviewed May 15, 2025

View reviewed changes

src/ndv/views/_wx/_array_view.py Outdated Show resolved Hide resolved

src/ndv/views/_wx/_array_view.py Outdated Show resolved Hide resolved

gnodar01 and others added 13 commits May 16, 2025 10:06

Revert visibility signaling

7abd03d

- reverts emit when setting lut visibility - reintroduces bug where all channels displayed on channel mode change - setup for alternative approach taht doesn't rely on emitting when setting visible

Set lut view state to keep visibility and display info

e5980fe

- allows remembering display and visibility setttings across channel mode changes

Set visible off when not displayed

9f5bec0

Show display options only after thresh

c59ce25

style(pre-commit.ci): auto fixes [...]

26e186f

Fix: no built-in for signal introspection

cb89ae7

Add initial display option tests

857aee6

Merge branch 'main' into wx-many-channel

f33711c

style(pre-commit.ci): auto fixes [...]

97601f2

Fix merge conflict remnants

6796096

Remvoe unused method

4aacff0

Put channel removal under test, and fix 0 check bug

f3d8360

Test channel selection popup

d375414

gnodar01 and others added 2 commits May 19, 2025 16:25

Test select [all|none]

66182ce

style(pre-commit.ci): auto fixes [...]

774c7c7

gnodar01 marked this pull request as ready for review May 19, 2025 20:33

gselzer reviewed May 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better gui for many channels (wx-specific) #195

Better gui for many channels (wx-specific) #195

gnodar01 commented May 13, 2025

codecov bot commented May 13, 2025 •

edited

Loading

gnodar01 commented May 13, 2025 •

edited

Loading

gselzer commented May 14, 2025

gselzer commented May 14, 2025

tlambert03 commented May 14, 2025

gnodar01 commented May 14, 2025 •

edited

Loading

tlambert03 left a comment

gnodar01 commented May 15, 2025 •

edited

Loading

gnodar01 commented May 19, 2025 •

edited

Loading

gselzer left a comment •

edited

Loading

gselzer May 20, 2025

gselzer May 20, 2025

gselzer May 20, 2025

gselzer May 20, 2025

gselzer May 20, 2025

gselzer commented May 20, 2025 •

edited

Loading

		if len(self._luts) < self._wxwidget._toolbar_display_thresh:
		self._wxwidget.toggle_lut_toolbar(False)

Better gui for many channels (wx-specific) #195

Are you sure you want to change the base?

Better gui for many channels (wx-specific) #195

Conversation

gnodar01 commented May 13, 2025

codecov bot commented May 13, 2025 • edited Loading

Codecov Report

gnodar01 commented May 13, 2025 • edited Loading

gselzer commented May 14, 2025

gselzer commented May 14, 2025

tlambert03 commented May 14, 2025

gnodar01 commented May 14, 2025 • edited Loading

tlambert03 left a comment

Choose a reason for hiding this comment

gnodar01 commented May 15, 2025 • edited Loading

gnodar01 commented May 19, 2025 • edited Loading

gselzer left a comment • edited Loading

Choose a reason for hiding this comment

gselzer May 20, 2025

Choose a reason for hiding this comment

gselzer May 20, 2025

Choose a reason for hiding this comment

gselzer May 20, 2025

Choose a reason for hiding this comment

gselzer May 20, 2025

Choose a reason for hiding this comment

gselzer May 20, 2025

Choose a reason for hiding this comment

gselzer commented May 20, 2025 • edited Loading

codecov bot commented May 13, 2025 •

edited

Loading

gnodar01 commented May 13, 2025 •

edited

Loading

gnodar01 commented May 14, 2025 •

edited

Loading

gnodar01 commented May 15, 2025 •

edited

Loading

gnodar01 commented May 19, 2025 •

edited

Loading

gselzer left a comment •

edited

Loading

gselzer commented May 20, 2025 •

edited

Loading