microscope-cockpit / cockpit

Cockpit is a microscope graphical user interface. It is a flexible and easy to extend platform aimed at life scientists using bespoke microscopes.
https://microscope-cockpit.org
GNU General Public License v3.0
37 stars 27 forks source link

adding a marked point will updated the mosaic canvas but not the displayed point list. #817

Closed iandobbie closed 2 years ago

iandobbie commented 2 years ago

The script I wrote using openCV to identify DAPI labelled nuclei processes the images and adds marked points in the mosaic. The mosaic updates its image canvas with the points however the point list is not updated until you click in that area.

Not sure what updates this part of the GUI but I guess it need to listen to the add point event and redraw.

carandraug commented 2 years ago

Where is that script and how is it adding the marked points? Or, how can I reproduce this issue?

I believe you're talking about the script in the cockpit paper. Looking at the paper, I found https://zenodo.org/record/5745648 and reading the script there I see that it's roughly doing:

from cockpit.interfaces import stageMover
from cockpit.gui.mosaic import window as mosaicWindow
[...]
                sitepos=[cellpos[0],cellpos[1],position[2]]
                stageMover.saveSite(stageMover.Site(sitepos,None,mosaicWindow.window.siteColor,
                                               size=mosaicWindow.window.crosshairBoxSize))
[...]

The issue can be reproduced by using cockpit's Python shell:

from cockpit.interfaces import stageMover
stageMover.addSite(stageMover.saveSite((0,0,0)))

Looking at the code for the mosaic window, I see that MosaicWindow.saveSite calls events.publish(events.MOSAIC_UPDATE) at the end.

iandobbie commented 2 years ago

Calling this doesn't seem to help, however once focus moves away from the mosaic window the listbox updates.

I have tried calling listbox.Refresh() and listbox.Update() but neither has any effect. I don't understand how to force this update without moving focus.

carandraug commented 2 years ago

The issue can be reproduced by using cockpit's Python shell:

from cockpit.interfaces import stageMover
stageMover.addSite(stageMover.saveSite((0,0,0)))

There are two typos on that code. It should have been:

from cockpit.interfaces import stageMover
stageMover.saveSite(stageMover.Site((0,0,0)))

Anyway, when I do this on PyShell, the site list on the "Mosaic View" window displays the new site immediately. I don't have the issue you're describing unless I'm misunderstanding something.

iandobbie commented 2 years ago

Yes, I have tried this and it doesn't reproduce the issue. The code from the findNuclei Script in the cockpit paper is shown above but for some reason it doesn't update the listbox until focus is moved. Attempts to reproduce this from the pyshell window seem to update fine. Maybe a threading issue? I wonder if a call in main thread decorator might fix it.

iandobbie commented 2 years ago

Ok, I added the following to the interfaces.stageMover module

@cockpit.util.threads.callInMainThread def saveSite(newSite = None):

So this forces the saveSite call to happen in the main thread and seem to immediately solve the problem. Of course also need the required import.

iandobbie commented 2 years ago

For me on a mac the issue is fixed with the commit above so closing.

carandraug commented 2 years ago

I don't think this is the right fix. The saveSite method itself has nothing to do with the GUI so it shouldn't be forced to run on the main thread. I see that saveSite publishes the new site event which the mosaic window subscribes to so I'm guessing it's the handling of that event that needs to be run in the main thread. We have cockpit.gui.EvtEmitter which is what should be used when a GUI needs to subscribe to cockpit events. It ensures that it runs in the main thread and does the unsubscription properly. Can you try that?

iandobbie commented 2 years ago

Good point. I have changed the mosaic window code to subscribe using the cockpit.gui.EvtEmitter for both new sites and deletions. This all seems to work.

Additionally I replaced the string event names with definitions for events.NEW_SITE and DELETE_SITE. All seems to work so closing once again.