Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: New ConfigGroupsEditor widget. #307

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

tlambert03
Copy link
Member

@tlambert03 tlambert03 commented Jul 4, 2024

i still find creating config groups to be a little clunky... playing around with another approach:

Screenshot 2024-10-14 at 10 48 44 AM

see examples/config_groups_editor.py

Copy link

codecov bot commented Jul 4, 2024

Codecov Report

Attention: Patch coverage is 29.80132% with 318 lines in your changes missing coverage. Please review.

Project coverage is 87.82%. Comparing base (d58003b) to head (e4deaf5).

Files with missing lines Patch % Lines
...mmcore_widgets/config_presets/_unique_name_list.py 23.97% 149 Missing ⚠️
...re_widgets/config_presets/_config_groups_editor.py 22.04% 145 Missing ⚠️
...idgets/device_properties/_device_property_table.py 60.65% 24 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #307      +/-   ##
==========================================
- Coverage   90.71%   87.82%   -2.89%     
==========================================
  Files          85       87       +2     
  Lines        9188     9622     +434     
==========================================
+ Hits         8335     8451     +116     
- Misses        853     1171     +318     

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

@tlambert03
Copy link
Member Author

tlambert03 commented Jul 4, 2024

hey @fdrgsp, would be curious to get your take on this.

While setting up station 8, I found the conventional micromanager multi-window process of setting up channel config groups cumbersome. and also, you frequently find yourself unchecking a bunch of device types to get at the state devices that typically hold the fluorescence filters.

So this widget is more of an "opinionated" arrangement of stuff that's designed to make it easier to get at the most important settings when creating config groups. for example, we prune the list of light path properties to those most likely to be used (while still allowing to show all if desired). i think you'll recognize the inspiration :)

could you run python examples/channel_config_widget.py and see how you like it? and if it can be improved?

@tlambert03 tlambert03 changed the title [wip] optical config widget feat: optical config widget [wip] Jul 4, 2024
@fdrgsp
Copy link
Collaborator

fdrgsp commented Jul 4, 2024

i think you'll recognize the inspiration :)

🤣

could you run python examples/channel_config_widget.py and see how you like it? and if it can be improved?

I definitely ❤️ it! create the channel configs is much easier! I think that I would need to try it on a real microscope to understand what can be improved or modified but I agree and I think this should be a widget.

@marktsuchida
Copy link

I like the dedicated combo box for Core-Shutter and hiding it in the list of properties. I think similarly the camera's Exposure could be hidden by default, as it almost never should be in the config group.

(I've also fantasized about a config editor with a 2D table: properties in rows and presets in columns. There could be a second pane on the left that just lists properties, without values, with checkboxes to select which properties to include in the group and hence the 2D table. Or the 2D table could be the whole window, with add/remove buttons for both properties and presets.)

@tlambert03
Copy link
Member Author

thanks for looking @marktsuchida :)

I've also fantasized about a config editor with a 2D table: properties in rows and presets in columns.

makes it a little harder to capture a case where one preset doesn't need to modify a specific property, right?

When I was first orienting myself to config groups, i found it slightly surprising that different presets within a group could actually have a different set of properties... but now actually I think it's quite powerful, and let's you really express the bare minimum number of things that must be set when switching to a new preset (while allowing non-critical things to just stay in whatever position they were last in)

In an effort, to make it a bit clearer which properties are selected, I've made the contrast a bit starker between a selected prop and an inactive one. and the preset names remain on the left

Screenshot 2024-07-10 at 11 16 59 AM

I think similarly the camera's Exposure could be hidden by default, as it almost never should be in the config group.

this one is kinda interesting actually. While it may feel like a very strange way to do things, the default way that Nikon Elements works on all of our stations is that exposure (and indeed many other camera properties) does go into the optical config settings. In elements, OCs are really much more "living/breathing things", such that when a user clicks on the button to switch to the red channel, and then changes the exposure as they are optimizing their image, those changes actually persist in that config (and then the user would not put a channel-specific exposure in the channels selector in the MDA widget as we and MMStudio (?) currently do).

not necessarily voting one way or the other, but it's interesting to think about.

@tlambert03
Copy link
Member Author

I've also fantasized about a config editor with a 2D table: properties in rows and presets in columns. There could be a second pane on the left that just lists properties, without values

a nice thing about having this all in python is that it would probably only take a couple hours for you to make an alternative config widget exactly how you want it and use it in your app :)

@marktsuchida
Copy link

makes it a little harder to capture a case where one preset doesn't need to modify a specific property, right?

True, though I think that could still be handled by an additional checkbox in each cell. Anyway, just putting the idea out there.

I was also surprised to see that the MMCore code is actually written to allow presets that don't have all the properties of the group. Loading such a config might break MMStudio, but that shouldn't restrict us, of course.

In elements, OCs are really much more "living/breathing things"

I think for this to work the config groups need to be composed of more intuitive parameters that all users can easily understand. We could maybe do this if we had something like a system of virtual properties that can (completely) hide the lower-level ones from the devices. Or the ability to cascade config groups. But an interesting way to do things for sure.

@tlambert03
Copy link
Member Author

tlambert03 commented Oct 14, 2024

ok, this PR is in a decent state now.

Screenshot 2024-10-14 at 10 48 44 AM

Things I haven't implemented but have discussed:

  • a "basic/expert" tab widget. The current PR simplifies a lot of things by using heuristics to highlight the "most likely" things that someone would want to edit: making the common use cases easy. But we also need to make sure to make the uncommon use cases possible. It may already be possible simply be checking the "show all properties" checkbox. But another option is to have an "expert" mode that simply gives the full device property table
  • the groupbox checkboxes are still unimplemented. The intention was for those to essentially gate a whole group of things at once (for example, unchecking the "camera" group box would mean that the preset excludes all camera settings... while making it easier for them to retoggle them all back on). I'm still unsure if this is good. Elements has this, but it might be confusing
  • all checked properties should be visible, regardless of whether the show all properties checkbox is checked (even if they would have been hidden by the heuristic).
  • TODO: Mouse wheel should not affect PropertyWidgets when they are embedded in a PropertyBrowser #372 is still annoying (but not part of this PR)

@tlambert03 tlambert03 changed the title feat: optical config widget [wip] feat: New ConfigGroupsEditor widget. Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants