napari-micromanager icon indicating copy to clipboard operation
napari-micromanager copied to clipboard

Properly connect gui state and core state

Open tlambert03 opened this issue 4 years ago • 2 comments

as observed by @ianhi in #20, the gui was written before there was a good way to receive callbacks from the core object. We need to go back and use event-based gui-state updates instead of on-demand updates.

tlambert03 avatar Jul 07 '21 18:07 tlambert03

to go back and use event-based gui-state updates instead of on-demand updates.

Is there a canonical place where state should live in a qt application? Or resources to read up on the reasonable flow of data

So when you say "instead of on-demand updates" does that mean that things like this shold be written differently?

https://github.com/tlambert03/napari-micromanager/blob/96a8a0cafeaac840fdf6a096b2b9cf4006554569/micromanager_gui/main_window.py#L344-L346

ianhi avatar Jul 07 '21 18:07 ianhi

Is there a canonical place where state should live in a qt application? Or resources to read up on the reasonable flow of data

In a basic QWidget-based setup (as opposed to a more abstract model/view setup) , the widget itself holds state. For instance (as you know) a QComboBox has a currentText property that you can get/set, but it's not intrinsically linked elsewhere.

Looking at the code again now, I see a lot fewer "on-demand" updates than I remember, but what I mostly meant were methods that we manually call to query the core and some gui state, rather than gui state that is directly listening to a signal emitted by the core object. For instance, all the _refresh_x methods, that get called by _on_system_configuration_loaded... I think (at least, it's worth exploring) that we should be connecting directly to the core onPropertyChanged event. So, after https://github.com/tlambert03/pymmcore-plus/pull/14 is merged:

In [1]: from pymmcore_plus import CMMCorePlus

In [2]: mmc = CMMCorePlus()

# just print anytime a property changes
In [3]: mmc.propertyChanged.connect(lambda *props: print(props))

In [4]: mmc.loadSystemConfiguration()
('Core', 'Initialize', '0')
('Core', 'Initialize', '1')
('Core', 'Camera', 'Camera')
('Core', 'Shutter', 'Shutter')
('Core', 'Focus', 'Z')
('Core', 'AutoShutter', '1')
('Objective', 'State', '1')
('Objective', 'Label', 'Nikon 10X S Fluor')
('Camera', 'Binning', '1')

In [5]: mmc.setProperty('Camera', 'Binning', 2)
('Camera', 'Binning', '2')

So, basically, we just need to map those (Device, Prop, Value) tuples to the corresponding widget set<Value> method ... something very roughly like this:

# where `self` is an instance of `MainWindow`
@mmc.propertyChanged.connect
def _update_gui(self, dev, prop, value):
    widget = self.get_widget_corresponding_to(dev, prop) 
    # unfortunately, the "value setter" method will depend on the widget type
    # so this is just an example for a combobox
    widget.setCurrentText(str(value))

and vice versa... anytime those widgets change, we should update the core. So a more declarative mapping might be useful:

CORE_MAP = {
    ('Camera', 'Binning'): (`bin_comboBox`, `setCurrentText`)
}

in which case the function above could be

@mmc.propertyChanged.connect
def _update_gui(self, dev, prop, value):
    widget_name, setter = CORE_MAP((dev, prop))
    widget = getattr(self, widget_name)
    getattr(widget, setter)(value)

but that's probably overly simplistic (since the device label isn't so constant). but that's generally what I was thinking

tlambert03 avatar Jul 07 '21 19:07 tlambert03