ome / omero-figure

An OMERO.web app for creating Figures from images in OMERO
http://figure.openmicroscopy.org
GNU Affero General Public License v3.0
15 stars 30 forks source link

Add All button for ROIs table #430

Closed will-moore closed 2 years ago

will-moore commented 3 years ago

Fixes #335

This provides an "Add All Shapes" button to add all the shapes that are on the current Z/T plane (or shapes with Z/T unset). It ignores shapes that are outside the current viewport.

To test:

Screenshot 2021-04-19 at 14 40 25

jburel commented 3 years ago

General point: What about shapes that are linked to a specific channel? Will the channel be turned on if the shapes are added and the channel is off?

pwalczysko commented 3 years ago

Discussed with @will-moore and identified several issues:

Bugs:

  1. There is no warning in case the single shape which you are attempting to add is outwith the viewport. Atm, every shape has a green enabled button, whether it is within the viewport or not, and this button is "clickable" even for shapes outwith the viewport, with no action and no message about the ROIs not being added
  2. Variation on the Bug 1. above, the Add All button does not have any warning either, in case there are no Shapes on the particular z or t plane - the button is active and "clickable", but no message about the futility of the action comes forth.

RFE:

  1. Explain somehow the fact that only 500 ROIs are loaded even when the "Add All" button is clicked, because of the loading by pages behaviour.

After consultation, both the bugs and the RFE could be helped a lot using a popup message after you have added a ROI saying "Added so-and-so many ROIs".

Also, it would be of value to ask on the original image.sc thread whether an "Add All" button which adds all ROIs from all z and t planes was meant, or, if the additon of the Shapes from the current z/t plane (as implemented now) is enough.

imagesc-bot commented 3 years ago

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/omero-iviewer-cant-display-more-then-500-rois/27830/4

will-moore commented 3 years ago

Improved messages for the "Add All" button, e.g: (also fixed the existing message for the "Add" button on each single Shape).

will-moore commented 3 years ago

As noted in a response on the original image.sc topic, adding Shapes ONLY on the current Z/T plane is sufficient.

will-moore commented 3 years ago

Simplest success message for Add All:

Screenshot 2021-04-22 at 15 49 34

As discussed, if Add All adds a Shape linked to a channel that is inactive, we turn on that channel(s). Also let the user know if we're only adding shapes from 1 page of ROIs and if some shapes are not added because they lie outside the viewport:

Screenshot 2021-04-22 at 15 42 32

To test the channels workflow, you can add theC to a shape (first finding the ID of a shape in iviewer) $ omero obj update Polygon:439071 theC=1 Then turn that channel off in figure before "Add All". Adding just 1 shape with theC set will also activate the channel but no dialog shown (same logic as for Z/T).

pwalczysko commented 3 years ago

Tested the Add and Add All buttons with shapes within and without the highlighted region, like the behaviour, the dialogs make sense.

The only tiny thing would be in the case where the "Add" fails to use

  1. RFE Could not instead of Couldn't

I will have a look at the channel behaviour on Monday.

pwalczysko commented 3 years ago

Tested the channels behaviour on https://merge-ci.openmicroscopy.org/web/webclient/?show=image-140171 user-3. All works as described, just the formulation on the dialog which reports about the ROIs addition could be improved.

Shapes linked to an inactive channel (2) were added, so this channel has been turned ON. -> As shapes linked to an inactive channel (2) were added, this channel has been turned ON.

pwalczysko commented 3 years ago

In summary, all works as expected here, just two RFEs

  1. https://github.com/ome/omero-figure/pull/430#issuecomment-825785950
  2. https://github.com/ome/omero-figure/pull/430#issuecomment-827475195
will-moore commented 3 years ago

That previous commit fails the build with //omero-figure/.omeroci/app-deps: line 6: npm: command not found. Reading https://linuxize.com/post/how-to-install-node-js-on-centos-7/ adding sudo yum install nodejs...

pwalczysko commented 3 years ago

Checked the messages, all good. The PR is green and the functionality is also as epected. Ready to merge fmpov.

jburel commented 3 years ago

@will-moore could you put the 4 commits starting from e866aeeedf37a0538e629634eaf528b99581b89f in a separate PR? They are not related to the feature worked on in this PR. This will be much easier to track down later Thanks

will-moore commented 3 years ago

@jburel Removed those commits from this branch and moved them to #435

jburel commented 3 years ago

Just an idea, we have example of images with large number of ROIs. Was it tested with a very large number of rois/shapes?

pwalczysko commented 3 years ago

Just an idea, we have example of images with large number of ROIs. Was it tested with a very large number of rois/shapes?

2362 ROIs in total on https://merge-ci.openmicroscopy.org/web/webclient/?show=image-140171 (but not all the ROIs are on the same plane)

will-moore commented 3 years ago

It's been tested on images with about 100 ROIs. It only adds the Shapes from the current page of ROIs, so the max we'd expect to handle is 500 ROIs, although the number of Shapes that are added could be more or less than this, since we only pick Shapes that are on the current Z/T plane, and then only if they are within the viewport.

pwalczysko commented 3 years ago

Did a new test with 150086 ROIs. The image https://merge-ci.openmicroscopy.org/web/webclient/?show=image-105665 is a large one. Lodded ROIs from 2 pages when zoomed in, and from one page (nr. 17) when fully zoomed out. Both worked, and the ROIs appeared as expected. (it takes some minute(s) to load the 500 ROIs, but it works)

pwalczysko commented 3 years ago

@will-moore

It's been tested on images with about 100 ROIs.

see https://github.com/ome/omero-figure/pull/430#issuecomment-849764161 and https://github.com/ome/omero-figure/pull/430#issuecomment-849780713

pwalczysko commented 3 years ago

The figure is https://merge-ci.openmicroscopy.org/web/figure/new/?image=105665

jburel commented 3 years ago

which user?

will-moore commented 3 years ago

user-3

jburel commented 3 years ago

For that image, the loading/adding of ROIs takes a long time. Will it be possible to add a spinner in that case? I moved to another tab to write comments, and the tab with the figure is now blank

jburel commented 3 years ago

Things are slowly coming back...

Screenshot 2021-05-28 at 16 23 54
jburel commented 3 years ago
Screenshot 2021-05-28 at 17 13 09

After clicking "add all shapes"

will-moore commented 3 years ago

I think those commits have made an improvement. Hopefully it will be a bit faster and should be less likely to crash now.

jburel commented 3 years ago