Ignore null ImageJ2 Overlays

[gselzer]

Suppose we have a Dataset, with a DefaultROITree as the rois property on the Dataset, that we want to display in the legacy UI.

To do this, we call LegacyUIService.show(<our dataset>), which will use the LegacyImageMap to create a synchronized ImagePlus. Once that is displayed, ImageJ Legacy will fire off a DisplayUpdatedEvent, which eventually triggers OverlayHarmonizer.updateLegacyImage. This method uses OverlayService.getActiveOverlay(<our display>), which returns null - this overwrites the DefaultROITree overlay.

This PR changes OverlayHarmonizer to only update the overlay if the ImageJ2 Overlay returned by OverlayService is not null - this prevents overwriting legacy Overlays unless we have an ImageJ2 Overlay that actually has content.

[hinerm]

@gselzer

I'm sorry; it's not clear to me what's happening. Here's my understanding:

1. You have a Dataset with a DefaultROITree attached - can you link me to the rois property you mentioned?
2. An ImagePlus is created, with its rois property populated with the DefaultROITree
3. The OverlayHarmonizer overwrites the rois property of the ImagePlus, setting it to null

Is that right?

If the Dataset has an attached DefaultROITree, why doesn't it report having an overlay? Is OverlayService.getActiveOverlay(<our display>) really doing the correct thing, if it's missing the attached DefaultROITree?

[gselzer]

@gselzer
1. You have a Dataset with a DefaultROITree attached - can you link me to the rois property you mentioned?

Yeah, that's checked here

2. An `ImagePlus` is created, with its [`rois`](https://github.com/imagej/ImageJ/blob/5addb75455a4fca940a15d1b7aa604ac36cf9079/ij/ImagePlus.java#L56) property populated with the `DefaultROITree`

Well, the DefaultROITree is converted to a ij.gui.Overlay, but yes, it is properly populated in the call to ij.ui().show(), before the events fire.

3. The `OverlayHarmonizer` overwrites the `rois` property of the `ImagePlus`, setting it to `null`

Is that right?

Yup!

If the Dataset has an attached DefaultROITree, why doesn't it report having an overlay? Is OverlayService.getActiveOverlay(<our display>) really doing the correct thing, if it's missing the attached DefaultROITree?

Well, there is no net.imagej.overlay.OverlayView on the associated ImageDisplay - I don't even know if the conversion logic exists, and I also don't know how well the net .imagej.overlay.Overlay logic is implemented - @ctrueden told me that the ImageJ2 Overlay work is superseded by the imglib2-roi work. We'd have to make a converter to be able to get an Overlay, created from the DefaultROITree, from the OverlayService - I don't know whether it's worth it, and I'm also not sure what the right thing would be...

[hinerm]

@gselzer ok I added a test.. does it look reasonable to you?

Currently it's failing because it isn't getting a display, which I am confused about because I thought IJLegacy could run headlessly..?!

I don't know whether it's worth it, and I'm also not sure what the right thing would be...

To me, here we are saying "The ROIService.ROI_PROPERTY of a Dataset is an overlay", so something else in the IJ2 framework needs to also think that. So what are the candidates for where that logic could go?

My thoughts:

• DefaultOverlayService - I guess this would require that when a Dataset is being displayed, its properties are interrogated and the ROITree is converted to an overlay? That's what you were implying, right?
• The OverlayHarmonizer - this would be a simpler fix to just check if the ImageDisplay's displaying a Dataset and if that Dataset has a ROITree attached..

[gselzer]

@gselzer ok I added a test.. does it look reasonable to you?

You'll need to ensure that OverlayHarmonizer.updateLegacyImage is being called like you said it does, once we figure out why it's failing. I can look next week.

[hinerm]

Currently it's failing because it isn't getting a display, which I am confused about because I thought IJLegacy could run headlessly..?!

First guess: try actually showing the dataset?

[gselzer]

@hinerm the test is now fixed, just had to add a couple extra services! Thanks to @ctrueden for the suggestion.