Skip to content

feat: Better config group editor #446

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

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

tlambert03
Copy link
Member

@tlambert03 tlambert03 commented Jun 25, 2025

This PR is a reconsideration of the code patterns in #307, but trying to achieve the same goal: A better Configuration Groups editor.

This design makes a proper QAbstractItemModel for configuration groups, modeling them as a tree of Groups -> Presets -> Settings (pulled out and merged in #447)

By doing so, we take advantage of synchronization across multiple views, and immediately get common Qt views for free, like QTreeView (on the right half of the image below) basically for free (added as a feature in #448)

Screenshot 2025-06-30 at 9 44 17 PM

Note

The main ConfigGroupEditor in this PR is only the part to the left of the Tree view on the image above, that image is just from the example that shows view synchrony

@marktsuchida, I would particularly appreciate your feedback here. not necessary to look at the code, but if you have a fork of this repo, just try:

gh co 446
uv sync
python examples/config_groups_editor.py

and then see how intuitive stuff is...
The goal is to:
A) prevent the need for a sequence of different windows as you create group -> select properties to include in a preset -> select the values for those presets ...
B) try to highlight the most "likely" properties that the user may be looking for (particularly when editing Channel groups), while providing a "show all" mode for more complex cases.

I know there's a few things missing still (but noting those things would be helpful too!)

@tlambert03 tlambert03 changed the title Q config Better config group editor Jun 25, 2025
Copy link

codecov bot commented Jun 26, 2025

Codecov Report

Attention: Patch coverage is 32.89902% with 206 lines in your changes missing coverage. Please review.

Project coverage is 85.91%. Comparing base (2a79de5) to head (b951b88).

Files with missing lines Patch % Lines
...ore_widgets/config_presets/_views/_config_views.py 21.61% 185 Missing ⚠️
...idgets/device_properties/_device_property_table.py 72.30% 18 Missing ⚠️
src/pymmcore_widgets/control/_rois/_vispy.py 0.00% 3 Missing ⚠️

❌ Your patch check has failed because the patch coverage (32.89%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #446      +/-   ##
==========================================
- Coverage   87.39%   85.91%   -1.48%     
==========================================
  Files         100      101       +1     
  Lines       11122    11411     +289     
==========================================
+ Hits         9720     9804      +84     
- Misses       1402     1607     +205     

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

@tlambert03 tlambert03 changed the title Better config group editor feat: Better config group editor Jun 26, 2025
@marktsuchida
Copy link

Lovely! Here are some initial thoughts I jotted down.

The Group and Presets lists are nice and clean, and the Duplicate button is awesome.

When I assume I don't have the right-hand-side tree view, I feel uncertain about the totality of properties in the selected preset. My first question was: where are the properties that are not of the camera or light path? It looks like everything that's not a camera or Core is considered Light Path, but that's not initially obvious.

I also don't have a mental model for what criterion is used when 'Show All Properties' is unchecked -- it seems like state devices' Label is included, but there are a couple others that seem device-specific?

I think the fact that the two tables of properties are not the flat list of all properties makes it harder to grasp what config groups and presets are (especially to someone new). But I do like the special handling of the Active Shutter and the hiding of shutter state by default.

Exposure does not seem to be hidden by default from cameras. I think we talked about this before and you were of the opinion that config groups could be something that is created/modified ad hoc. But I think the MDA widget currently handles Exposure separately. On the other hand, I'm not sure it is worth adding a Show All Properties checkbox just for the purpose of hiding Exposure. Maybe it's simpler for the MDA to warn if there is a conflicting Exposure setting in the presets, or just silently override them.

What about the Core properties other than Shutter and Camera? I think it's correct that these don't make sense normally, but what would happen if a config is loaded that contains them?

Also with the Camera having many properties (same could happen with Light Path), it would be nice if there were a way to hide the unselected properties (or perhaps the properties that are not selected in any preset of the selected group), so that one can see the full definition of the selected preset on the screen at the same time (without scrolling).

When I created a new group (without duplicating), the property selections from the previously-selected group and preset remained displayed. Probably the property tables need to be disabled (or hidden and replaced with the message "no preset selected") when no preset is selected.

When creating a new group or preset, it might be good if the name of the newly created item is selected and focused, so that one can immediately type in the name.

I think pre-init properties really shouldn't be selectable, because setting them post-init is not safe in general (or should MMCore enforce read-only for these?). On the other hand, read-only properties may be useful to optionally display, because they may display computed information based on the values of other properties (e.g., camera readout time) -- but maybe it's better to leave them out to keep things simple.

I think it might be helpful to have a way to close-and-revert-changes, in case one messes things up. Or just an "undo changes" button not tied to closing the window. Maybe. But if this window can be left open for a long time, maybe those don't really make sense.

…ConfigGroupsModel

fix: update icon names for AutoFocus and Magnifier device types
refactor: enhance PropertyValueDelegate and ConfigGroupsEditor for better structure and functionality
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.

2 participants