data-menu: implement action bar (delete, edit subset, metadata)

[kecnry]

Description

This pull request implements the bottom action bar (acting on the selected data/subset items in the data-menu) to view metadata, remove from viewer, remove from app, and modify subsets.

NOTE: this currently includes the diff from #3263, and so should be rebased on main after that is merged and merged second. The diff should be able to filter out the commits from #3263 if reviewers want to review in parallel.

Screen.Recording.2024-10-31.at.3.27.47.PM.mov

Screen.Recording.2024-10-31.at.3.28.18.PM.mov

Until finished and officially exposed, the data-menu must be enabled via:

for viewer in viz.viewers.values():
    viewer._obj.data_menu._obj.dev_data_menu = True

TODO:

• look into test failure and decide if in scope
• forbid deleting cube, etc, from app as per old data-menu

Change log entry

• Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
  list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
  should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

• Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
• Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
• Do the proposed changes follow the STScI Style Guides?
• Are tests added/updated as required? If so, do they follow the STScI Style Guides?
• Are docs added/updated as required? If so, do they follow the STScI Style Guides?
• Did the CI pass? If not, are the failures related?
• Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[kecnry]

@bmorris3 - can you confirm if this is correct? I ran into this bug when adding test coverage (where it would complain otherwise that data_label == '' was not available as new_parent). Do you think this should be split out to a separate PR and backported?

[bmorris3]

Yes, I actually just encountered this yesterday too! I was experimenting with @eteq's unloading-data questions and was trying to untangle the bug. It looks like this is probably what we need. Testing this out now.

[kecnry]

I'm not sure if the cause of this is the same as the other known state delayed bugs or not, but do see the choice dissappear when testing locally in the notebook, so I suspect this is probably caused by the async nature of app.remove_data_from_viewer. We can either track that down here or consider it as a follow-up since I don't think its directly related to any new code added in this PR.

[rosteen]

In the notebook, I don't see the selected subset disappear from the menu after removing from app until after I click somewhere else - that's probably why this is failing.

[rosteen]

Actually, the same thing happens with remove from viewer - the eye icon doesn't change to toggled invisible until after I click somewhere else.

[kecnry]

ok, probably related to the subset state bug you're investigating then, do you think? We can probably re-enable this as a regression test for that then?

[rosteen]

Hmm, likely. I think that's an ok plan for now.

[codecov]

Codecov Report

Attention: Patch coverage is 85.82090% with 19 lines in your changes missing coverage. Please review.

Project coverage is 88.76%. Comparing base (80298a3) to head (e855110).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...viz/configs/default/plugins/data_menu/data_menu.py	85.03%	19 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3264      +/-   ##
==========================================
- Coverage   88.78%   88.76%   -0.03%     
==========================================
  Files         125      125              
  Lines       18832    18956     +124     
==========================================
+ Hits        16720    16826     +106     
- Misses       2112     2130      +18     

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

[javerbukh]

Could not find any unexpected behavior during testing and code looks good. I'd be interested if some of the subset creation or modification could be handled by the subset plugin API down the line but I understand wanting to keep everything in one place for the time being.

[rosteen]

Very nit-picky on the phrasing, but it bothers me a little bit that for data, this actually removes the data from the viewer, but with a subset, it just makes it invisible. Could we have conditional text here to reflect what is actually happening in the subset case? I know that's extra complicated since you could select both a subset and a data layer, this isn't a deal breaker for me.

[kecnry]

yeah, agreed. We could handle this with the text and/or tooltip changing based on the selection or we could disable it entirely if a subset is selected. @Jenneh - thoughts?

[Jenneh]

@rosteen what do you think of updating the tooltip for subsets to say "hide from viewer" rather than "remove selected data"? would that be enough to clarify?

[kecnry]

there could also be three versions of the tooltip text that change based on the selection:
data only: "Remove selected data from this viewer only"
subsets only: "Hide selected subsets in this viewer"
data+subsets: "Remove selected data and hide selected subsets in this viewer"

(but I'd vote to maybe keep the button text fixed since it maps to the API method)

[kecnry]

how is this?

Screen.Recording.2024-11-07.at.1.05.11.PM.mov

[rosteen]

That looks good to me!

[bmorris3]

Just recording here for future reference – if you delete all data entries from an Imviz instance, using the new data menu features, the old data menu features, or the API, you get the following traceback because there are no layers:

---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
File ~/miniconda3/envs/jdaviz-only-7/lib/python3.11/site-packages/ipywidgets/widgets/widget.py:773, in Widget._handle_msg(self, msg)
    771         if 'buffer_paths' in data:
    772             _put_buffers(state, data['buffer_paths'], msg['buffers'])
--> 773         self.set_state(state)
    775 # Handle a state request.
    776 elif method == 'request_state':

File ~/miniconda3/envs/jdaviz-only-7/lib/python3.11/site-packages/ipywidgets/widgets/widget.py:650, in Widget.set_state(self, sync_data)
    645         self._send(msg, buffers=echo_buffers)
    647 # The order of these context managers is important. Properties must
    648 # be locked when the hold_trait_notification context manager is
    649 # released and notifications are fired.
--> 650 with self._lock_property(**sync_data), self.hold_trait_notifications():
    651     for name in sync_data:
    652         if name in self.keys:

File ~/miniconda3/envs/jdaviz-only-7/lib/python3.11/contextlib.py:144, in _GeneratorContextManager.__exit__(self, typ, value, traceback)
    142 if typ is None:
    143     try:
--> 144         next(self.gen)
    145     except StopIteration:
    146         return False

File ~/miniconda3/envs/jdaviz-only-7/lib/python3.11/site-packages/traitlets/traitlets.py:1510, in HasTraits.hold_trait_notifications(self)
   1508 for changes in cache.values():
   1509     for change in changes:
-> 1510         self.notify_change(change)

File ~/miniconda3/envs/jdaviz-only-7/lib/python3.11/site-packages/ipywidgets/widgets/widget.py:701, in Widget.notify_change(self, change)
    698     if name in self.keys and self._should_send_property(name, getattr(self, name)):
    699         # Send new state to front-end
    700         self.send_state(key=name)
--> 701 super().notify_change(change)

File ~/miniconda3/envs/jdaviz-only-7/lib/python3.11/site-packages/traitlets/traitlets.py:1525, in HasTraits.notify_change(self, change)
   1523 def notify_change(self, change: Bunch) -> None:
   1524     """Notify observers of a change event"""
-> 1525     return self._notify_observers(change)

File ~/miniconda3/envs/jdaviz-only-7/lib/python3.11/site-packages/traitlets/traitlets.py:1568, in HasTraits._notify_observers(self, event)
   1565 elif isinstance(c, EventHandler) and c.name is not None:
   1566     c = getattr(self, c.name)
-> 1568 c(event)

File ~/git/jdaviz/jdaviz/configs/default/plugins/data_menu/data_menu.py:188, in DataMenu._dm_layer_selected_changed(self, event)
    184 with self.during_select_sync():
    185     # map index in dm_layer_selected (inverse order of layer_items)
    186     # to set self.layer.selected
    187     length = len(self.layer_items)
--> 188     self.layer.selected = [self.layer_items[length-1-i]['label']
    189                            for i in self.dm_layer_selected]

File ~/git/jdaviz/jdaviz/configs/default/plugins/data_menu/data_menu.py:188, in <listcomp>(.0)
    184 with self.during_select_sync():
    185     # map index in dm_layer_selected (inverse order of layer_items)
    186     # to set self.layer.selected
    187     length = len(self.layer_items)
--> 188     self.layer.selected = [self.layer_items[length-1-i]['label']
    189                            for i in self.dm_layer_selected]

IndexError: list index out of range

[kecnry]

Thanks - I can reproduce (and saw that once in another PR, but thought it was introduced there and not here). I think that should be fixed here... and if non-data-menu logic fails when deleting everything then we can instead block deleting the last data entry.

[kecnry]

@bmorris3 - that case should be fixed for now - seems it was a bit random based on order messages were processed and since its only updating the tooltip, I just added a workaround so it doesn't raise an exception.

[rosteen]

LGTM now, thanks.