Refactoring metadata classes

[carlosmariorr]

Refactoring the metadata widget

Background

The current version of the metadata widget creates a user-generated entry in the metadata parameter of the napari layer. As the layer parameters evolve, the metadata can now be set directly to the layer without the necessity of the user-generated entry.

Objective

Refactor the code so that the metadata is manipulated by the plugin widget and set directly to the layer parameters.

Brief summary of the changes:

Removed code (Yanking galore):

• Remove the example imports and references in the main code. (Not the directories nor examples themselves, just the references in the __init__.py and the napari.yaml (The directories and example images themselves are still somewhere...).
• Removed the mentioned example files and directories).
• Removed the _axes_widget.py file that includes the classes used in the previous version of the widget.
• Removed the channel from the axis types (Only space, time and string left). Channels seemed useful for categorizing the axis but since the type is linked to a pint unit, I'm not sure how to implement it. Ideas are welcome.
• Removed no longer used code in the _model.py.
• Removed the reader/writer files.
• Removed the specific units combo box files (e.g. _spatial_units_combo_box.py) used in the previous widget version.
• Removed the previous transform widget.
• Removed the unused _widget_utils.py file.

Added code:

• Added methods to get and set the metadata to the _model.py so that the GUI only passes the layer references instead of directly setting the values.
• Added a couple of units to the space category, including the "pixel" unit which is the default unit set by napari? <- Need reference here
• Added the contains classmethod to the spatial and temporal units to quickly find out if a given unit string is part of the units set. (Files: _space_units.py and _time_units.py)
• Added the MetadataComponent and the AxisComponent protocols to generate the widget. This way, the new widget can be extended by just adding the component class with the decorator and creating the functions to be called by any added QWidget in the MetadataWidget class (Main widget added by the plugin).
• Added the ability of the dock widget to detect its orientation when docked and arrange its layout between vertical and horizontal. TODO: Need to add QScrollWidgets for every entry in the dock widget so that it actually fits in the screen when images have too many axes of with small resolution monitors (Even 1080p won't fit all widgets expanded).
• Added the ability collapse the elements of the MetadataWidget (General File Metadata, Axis Metadata and Inheritance widgets) by pressing a button.
• Added the ability to inherit the axes metadata from one image to another using the inheritance checkboxes that are added to every AxisComponent.

Sacrifices

The plugin has some readers/writers and example metadata that can no longer be used. They were great implementations but probably need to move them to another plugin / rework them.

[andy-sweet]

Thanks so much for this massive update! This plugin was written a long time ago and for some very specific goals, so it makes sense that it would need some big changes.

I'm mostly checking in to make sure that you're not waiting on me for a review here, since I transferred this to the napari organization (see #67 for details). I'm no longer a napari core developer and I'm not planning to maintain this particular plugin either.

[TimMonko]

Hi @andy-sweet , thanks for stopping by!
It's no surprise you might be confused. @carlosmariorr started this work at the Hackathon in Japan, so we are aware of it's existence and the core team (namely at least me and @DragaDoncila) will take care of it. Thanks for the core work on this originally. Despite having to rip out so much, your architecture made a helpful core framework.
Excited to review this as soon as pneumonia stops dragging me down.

[andy-sweet]

@TimMonko : thanks for clarifying! Feel free to mention me on issues and PRs if there's any specific feedback anyone needs. Hope you feel better soon!

[TimMonko]

@carlosmariorr, I merged the changes after #69 in, so the package is now modernized and ready to go, in that sense. I also deleted vestigial test files, but my brain power is running out on fixing up test_widget.py for now.

My initial goal is complete, wherein I just wanted to b e in a ready place to review this, will all the other things sorted and out of the way. Now that such is done, I'll start working on reviewing the code changes. Most of the files removed make sense, but I am a bit concerned about moving some of the logic out of other modules and all into widget.py

The widget does work when I play with it, as much as I can. A few UI quirks, but UX seems ok. So for that matter, I think the goal now is to just clean anything that's a big sore thumb out of the way, and then get this properly tested so we can merge this. After that we can follow up on quirks / refactor / etc.

Great work so far!

[carlosmariorr]

Thank you a lot @TimMonko I'm back from my holidays and ready to dive into this again.

[TimMonko]

Brief notes from community meeting today

• I ran a nice demo of it and its working great, including with plugins that read in lots of metadata
• Works with linked layers BEAUTIFULLY!
• We need to make sure we only fix blockers, not quirks for follow-ups
• We need unit tests
• We need to fix the GUI issues, with size policy stuff and having scrolls
• Carlos will make a running issue in napari-metadata with our to-dos so they don't all end up hidden later in the PR
• But this is really close to MVP! our goal is to ship first alpha by end of Jan 16th

Plan is that both Carlos and I will push on this. I'll start by refactoring widget.py a bit, and then building up some unit tests. Carlos is going to look at his todo list from 2025-11-27/28 meeting, which I'm going to copy here:

• very nice dropdown, WHICH CAN BE BEAUTIFULLY DRAGGED TO THE TOP/HORIZONTAL AND ARRANGES BEAUTIFULLY
• played a bit with scrollable container but needs more attention, was being screwy
• inheriting metadata
  • carlos/juan considers that inheritance could be a separate widget
  • tim/draga encourage, for now, that the current widget setup be remained (edit today: lol what does remained mean...)
  • consider a "tooltip" that says "checked properties are inherited from the inheritance lanyer" <- put this in the "Axes Inheritance" widget (which could be renamed to "Inheritance Widget")
  • "Set Current Layer" -> change to dropdown of layers
  • "Apply" button -> "Inherit Properties to Selected Layer(s)" button
  • inheritance of "batch" style, should it be a separate widget, or should it be the "shared" list aka the layer list. Tim has experience with both styles, and found that users find the experience of using the layer list to be more intuitive compared to <- Jan 8th: this is working with linked layers!
• consensus to rip out reader and writer
• consensus to rip out the sample data

[carlosmariorr]

Okay, I think the Horizontal and Vertical containers are fixed now with proper scroll areas and I also put them into separate files so that the _widget.py file is not saturated with classes. The old vertical and horizontal container classes were deleted too. I think the GUI layout and buttons should be very close to the final form I envisioned. I'll show them in tomorrow community meeting for comments.

Following this I'll go ahead and tackle the Inheritance widget and system (The checkboxes and labels and selection indicators). Hopefully completed tomorrow so we can focus on the tests to complete the alpha.

I'll make the issue with the TO DO list later so we know what's next in following PRs.

[TimMonko]

Great work @carlosmariorr !!! The refactor looks great so far. 🥳 💯
Just going to do a drive-by for the moment

The scrolling and properly inheriting theme colors is 🔥

When opening the widget prior to adding any layers, the widget does not obviously contain the inheritance boxes, but does scroll nicely!

I wouldn't worry about this until we change up the inheritance GUI, but wanted to note we should check that situation.
With layers open first and then widget, it expands appropriately though!

I don't think this is a big deal though because it didn't break anything. It's really just an issue with the float slider, I think. In fact, it seems to be an issue with the napari QtDimSliderWidget so just noting here for now lol.

Something I want to think about is how we should actually label the axes. This just looks a bit off to me

Maybe we should just stick to the same order as it's indexed and use
ax0,ax1,ax2. I remember you gave a reason for why you used the numbers you did. The - is throwing me off, is that supposed to be minus indexing or a separator?
This feels less overwhelming and confusing to me to be labeled ax or axis

Horizontal container is scrollable vertically, but I dont see a scroll bar so its not intuitive. I wonder if we should set the size policy by default to fit the full view of the containers, but allow them to be squished like you've done with teh scroll bars.

I'll probably crack on this a bit in about 12 hours too, at the least I want to move tests to root and start organizing them. I didn't want to previously because the merge conflict would make me 🤮

[carlosmariorr]

@TimMonko thanks~ I'll not be able to work on this until I'm done with lab work tomorrow but I'll keep the commits short so we can track progress easy.

About the following:

Maybe we should just stick to the same order as it's indexed and use
ax0,ax1,ax2. I remember you gave a reason for why you used the numbers you did. The - is throwing me off, is that supposed to be minus indexing or a separator?

There's nothing we can do about it. Those labels are literally the property labels that napari assigns to the layers when it can't read or there's no data about dimension labels. This is literally the correct thing to do for now. We don't want to display different labels than what is in the layer and we definitely don't want to just change the labels by just opening the plugin. I know that some people commented on this but personally I think it is the correct way of displaying just because it is the way that dimensions are ordered in the multidimension array of the data. So yes, the "-" stands for the minus indexing of the array.

If we want to change those defaults, we need to change the layers defaults in napari,

And about this:

Horizontal container is scrollable vertically, but I dont see a scroll bar so its not intuitive. I wonder if we should set the size policy by default to fit the full view of the containers, but allow them to be squished like you've done with teh scroll bars.

Yeah, that was my mistake, because the horizontal is a copy of the vertical containers, I didn't update the entire behaviour of the QWidgets that actually have the contents. My goal was this:

-Vertical widget vertical scrolling is controlled by the main container (Individual sections don't do vertical scrolling).
-Vertical widget horizontal scrolling is controlled by each section (Because sections are different widths so we don't want to scroll ALL sections if we just want to see the units or of an axis because that would move the general file metadata out of the viewport).
-Horizontal widget horizontal scrolling is controlled by the main container (Individual sections don't do vertical scrolling... <-- I didn't disable this so right now that's the only way to scroll vertically I believe, also, no bar is shown because my plan was to disable this type of scrolling).
-Horizontal widget vertical scrolling is controlled by the individual sections, same reasoning as the horizontal scrolling in the vertical widget.

I thought that would be the optimal way of scrolling but I'm not sure if you have other thoughts...

[TimMonko]

@carlosmariorr, I took the liberty to combine your beautiful collapsible containers into one class because I was realizing how much was duplicated. Even then, with having to case things for horizontal vs vertical, for me it was easier to compare this way. I'm giving you the option of accepting this, or you can just revert my commit completely 😁
Then, I'm currently working through abstracting some of the layout from the MetadataWidget into its own module, with the help of an LLM. I'm very close to having it ready, and it actually makes some of the scrolling stuff work even more nicely. I'll probably submit that as a PR to your fork though, since its a bigger logic change. I'm too tired rn though 🙃

[jni]

I responded on the napari issue about the negative axis labels:

napari/napari#8558 (comment)

[carlosmariorr]

I took the liberty to combine your beautiful collapsible containers into one class because I was realizing how much was duplicated.

@TimMonko I was just about to tell you that this needed to happen because I was just testing and they're simple copy paste of each other with small logic changes that can be passed as flags or something.

Thanks!

[carlosmariorr]

I just changed the inheritance widget to its own file, upgraded the components to be more legible and moved some of the logic to the _model.py when I thought it made sense (For connecting the events and getting the layers, etc. etc.)

I am missing only two things for this to get to my goal:

1. connect the appearance of the checkboxes to the expanded/collapsed state of the inheritance widget.
2. move the method to apply the inheritance and connect it to the widget's button.

I'll do that tomorrow's morning.

[TimMonko]

I just changed the inheritance widget to its own file, upgraded the components to be more legible and moved some of the logic to the _model.py when I thought it made sense (For connecting the events and getting the layers, etc. etc.)

Its looking good! I like the new translations

I am missing only two things for this to get to my goal:

1. connect the appearance of the checkboxes to the expanded/collapsed state of the inheritance widget.

What's your idea / urgency for this? it works for me as is pretty much. I mean we do need some kind of pointer about the purposes of the check boxes, but your idea sounds like it could be a follow-up to me!

---

Going to start working on tests. I want to refactor, but we can do that in the future. Unless the unit test becomes too busy without abstracting, then I'll make changes. I think we can really focus on getting this done soon, so that we can get an alpha ready

[TimMonko]

Suggested change

	self._apply_button = QPushButton('Apply')
	self._apply_button = QPushButton('Inherit Checked Metadata')

How about something like this phrase? This way it lets users know the purposes of the checkboxes. Also, the more general "checked metadata" helps us be flexible to renaming things so that its not just "Inherit Axes Checked Metadata" for example

[carlosmariorr]

@TimMonko Yes, right now I'm not going to implement the checkbox states. Right now the only thing missing is to reconnect the apply function of the inheritance widget. It got a bit more complicated that just moving the function but I had the commit 90% done. I'm very busy this afternoon so I won't be able to finish it after I get back home. I'll push it tonight and I think that's all we need for the alpha right?

[TimMonko]

@TimMonko Yes, right now I'm not going to implement the checkbox states. Right now the only thing missing is to reconnect the apply function of the inheritance widget. It got a bit more complicated that just moving the function but I had the commit 90% done. I'm very busy this afternoon so I won't be able to finish it after I get back home. I'll push it tonight and I think that's all we need for the alpha right?

Sounds great! I think so too. Double check I didn't break anything though. I had a pretty derpy refactor of _model.py by creating resolve_layer to handle a lot of the duplicated logic. This came about when I created the test file - -because I noticed the repeating patterns. _model.py should be nicely tested now, which is at least a start! I'll work on some of the smaller files, and then work my way up to the widget.

However, I don't think we should delay on that, I just wanted proof to myself we can get tests going again. Going to delete test_widget.py for now to see if tests are looking ok in CI.

I'm going to message you on Zulip, and we can talk about making the alpha and look through things together. Perhaps you could even make the release. Not sure where the release state is with the changeover -- might need to update pypi and such. But, we might as well learn together :)

[codecov]

Codecov Report

❌ Patch coverage is 68.75000% with 120 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.02%. Comparing base (fa2ad93) to head (ebcf067).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/napari_metadata/_collapsible_containers.py	67.93%	42 Missing ⚠️
src/napari_metadata/_inheritance_widget.py	64.70%	42 Missing ⚠️
src/napari_metadata/_model.py	67.92%	34 Missing ⚠️
src/napari_metadata/_space_units.py	83.33%	1 Missing ⚠️
src/napari_metadata/_time_units.py	91.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #68       +/-   ##
===========================================
- Coverage   94.38%   43.02%   -51.37%     
===========================================
  Files          20       10       -10     
  Lines        1586     1871      +285     
===========================================
- Hits         1497      805      -692     
- Misses         89     1066      +977     

☔ 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.

[carlosmariorr]

Ok, I just reconnected the apply inheritance function to the inheritance widget. I tested by myself for a little and it is working fine.

I did encounter a bug with the new template layer selection method (Combobox method)
When I rename a layer the combobox is not updated with the new name because is not a layer list event. So we need to add the listener to individual layers? Is that the best option? Anyhow, it is not super priority as it should not break anything. The combobox actually stores the layer reference and displays the text so even if the user selects, changes layer name and then applies, the inheritance should still be true.

I did create a new file called _protocols.py
I'm not sure if that's the best place to store them but I do need some references to the AxisComponent Protocol in both _widget.py and _inheritance_widget.py so I needed them outside on their own. Right now I'm not using the AxisComponent protocol in the inheritance widget but the plan is to move the logic of the apply inheritances to the _inheritance_widget.py from _widget.py so this is just the first step.

The function of apply inheritance currently only works with the current layer. This is OK because of how the GUI is implemented but for the future we probably want to set it up so that you can pass the template and inheriting layer. Should be easy but that moves to a future PR probably.

I'm going to message you on Zulip, and we can talk about making the alpha and look through things together. Perhaps you could even make the release. Not sure where the release state is with the changeover -- might need to update pypi and such. But, we might as well learn together :)

Great! I'll be waiting for the message. Thanks @TimMonko !!!

[TimMonko]

We are aware that the files have not been fully tested. But we are merging in order to make an alpha and start trying to integrate into napari for pre-releases