-
Notifications
You must be signed in to change notification settings - Fork 21
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: add group and preset table widget #122
Conversation
Codecov Report
@@ Coverage Diff @@
## main #122 +/- ##
=======================================
Coverage ? 84.60%
=======================================
Files ? 30
Lines ? 2689
Branches ? 0
=======================================
Hits ? 2275
Misses ? 414
Partials ? 0
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A great start! I think most of the remaining work is on cleaning up _set_widget. Here's what I would recommend. Make the entirety of the table code something like this:
class MMGroupPresetTableWidget(QtW.QWidget):
def __init__(self):
super().__init__()
self._mmc = _core.get_core_singleton()
self._mmc.events.systemConfigurationLoaded.connect(self._populate_table)
self.table_wdg = MainTable()
self.table_wdg.column_headers = ("Groups", "Presets")
self.table_wdg.show()
self.setLayout(QVBoxLayout())
self.layout().addWidget(self.table_wdg.native)
def _populate_table(self):
self.table_wdg.clear()
if groups := self._mmc.getAvailableConfigGroups():
self.table_wdg.value = {
"data": [[group, make_group_widget(group)] for group in groups],
"index": [],
"columns": ["Groups", "Presets"],
}
and then work on creating a new function make_group_widget
that just takes the config group name (one can get presets from that) and builds a widget. (mostly using your _set_widget
code). It still needs some work though. It was a little bit hard to parse the intentions of each conditional in there, so a general docstring and comments for the whole function would be greatly appreciated. It should state the heuristics it uses (such as only looking at the first preset in the group), and briefly describe what happens if there's a single device in the preset, multiple devices, or no device. Lastly, the _on_change
function itself shouldn't be making all of those isinstance()
checks: we want callbacks to generally be fast, and you already know what type of widget it is... so do something like:
if isinstance(wdg, ...):
def _on_change(value: Any): ...
elif isinstance(wdg, ...):
def _on_change(value: Any): ...
wdg.changed.connect(_on_change)
super().__init__() | ||
|
||
self._mmc = _core.get_core_singleton() | ||
self._mmc.events.systemConfigurationLoaded.connect(self._populate_table) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there anything we need to do on events.configGroupChanged
or events.configSet
to keep the table in sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I think we should add a function that after receiving the events.configGroupChanged
or events.configSet
check if there is the group in the table and change the wdg.value
(with blocksignal
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agree that we should be updating based on those signals. One thing to consider is what to do when we get a configGroupChanged event with a blank string for a preset. That can happen if the system state is changed such that it doesn't correspond to any preset (e.g. contains exposure and exposure was changed). micromanager takes the approach of just putting a blank value in the dropdown which I always found to be confusing. In fact I actually thought it was a bug for a long time.
What do oyu think about doing the following when receiving a configGroupChanged
signal with a blank string for the preset:
Change the visual indicators on the preset combobox to indicate that it used to be something but that that no longer exactly represents the state of the scope. I'm thinking something along the lines of graying out the text a little, or a red background, or maybe a strikethrough and also adding a tooltip that explains the situation.
I wonder if we can be a bit more expressive in the UI here than micromanager is. In micromanager you can set a config group, but then if you change a property so that the system state no longer reflects that preset then the preset dropdown goes blank which was always a source of confusion for me. An easy way to end up there is if you put the exposure value into a preset. I now know that you shouldn't, but I suspect I'm not alone in having done that. The consequence is that if then change the exposure the configGroupChagned` signal will fire with an empty string as the preset.
So there's this splitting of state between what the user set, and what the actual state of the system is. I think we can represent that by instead of just showing preset names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also the discussion of the two config signals here: https://github.com/tlambert03/pymmcore-plus/pull/38#issue-962948450 I think this is the relevant use case of the configSet signal. You set the config, but due to trickiness of what the devices do inside themselves the actual changed signal that gets fired is blank. Unless we are also listening to the set signal then you can up with the bad user experience of selecting something from a dropdown and having it immediately switch to blank.
def _on_system_cfg_loaded(self): | ||
self._populate_table() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this used anywhere? or not, since you directly connect the event to populate_table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for now it is only called when the cfg is loaded
looks and feels lovely! so nice to see all of state across all of the widgets start to be very consistent :) it's working! here's a very minor annoyance with the dropdown. I'm not even really sure how to fix this, but I suspect it will bug you too and maybe you'll figure it out 😂 d.mov |
@tlambert03 I think I fixed this annoying issue with _set_combo_view 😂 |
I see you moved the tab order back :) ... I'm gonna move it again. I don't like having the snap tab and the MDA tab (both related to acquisition, just different n dims), separated by a config-like tab. It's the least like the other tabs and I think it needs to be over on the right. maybe later we place it elsewhere entirely? |
merge after #164
This PR adds to the gui a table widget (
_group_preset_table_widget.py
) that contains the group and presets retrieved from the loaded system configuration.I also added 3 new groups to
test_config.cfg
for tests (_combobox_no_preset_test
,_lineedit_test
,_slider_test
).