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

Rois pagination #422

Closed will-moore closed 3 years ago

will-moore commented 3 years ago

Fixes #334

Adds support for loading ROIs by page in the main Edit ROIs dialog, as well as the Crop dialog.

To test ROIs dialog...

Screenshot 2021-01-31 at 23 43 51

To test Crop dialog...

Screenshot 2021-01-30 at 22 53 16
pwalczysko commented 3 years ago

Works as described in the PR header. Tested on multi-z image with > 2000 ROIs on different z planes. https://merge-ci.openmicroscopy.org/web/webclient/?show=image-140172 (user-3)

Couple of RFEs, which might or might not be dealt with in this PR, I think up to @will-moore

  1. Crop dialog rectangles: One has load al the ROIsl to find out if there were any rectangles already (there were none, and I have drawn some in iviewer).These new rectangles were added to the bottom of the ROI list, and loaded only when I clicked 10 times on the Load ROIs button. Once I added a rectangle, it worked, and the crop was fine. But then I realized this is not an optimal rectangle, and I had to go back to the Crop dialog. All the ROIs were unloaded - so I had to again click 10 times to load the rectangles and select another rectangle. - suggestions: When I am in Figure, could the ROIs stay loaded once I loaded them into the Crop dialog ? Could there maybe be a function saying "there are no Rectangles here" instead of me loading all the ROIs just to find out there are no rectangles ?
  2. Edit ROIs dialog: The preview is actually much less forgiving that the Crop dialog one. The ROIs are viewable only when hovered over in the list. There is no way to order the loaded ROIs according to z, so a mixture of the ROIs on different z planes in the same place of the table is the result. There is a pending RFE about Add all ROIs button, but I would think also Add all ROIs from z plane would be most useful too.
will-moore commented 3 years ago

Thanks:

will-moore commented 3 years ago

Now we load only ROIs with Rectangles for the crop dialog. That should fix RFE 1) above.

pwalczysko commented 3 years ago

Now we load only ROIs with Rectangles for the crop dialog. That should fix RFE 1) above.

@will-moore thank you , the RFE 1) above fix works as expected, it seems that it even nicely keeps the loaded rectangles there when repeated Crop dialog usage is done.

I have one more RFE Re: teh ROI Edit dialog. The info about the ROIs to be loaded could be on the top, not on the bottom (the bottom is actually not seen at first, only when you scroll there). This would spare the user to wonder "where are my ROIs ?" because they did not load them all as they do not know there are not all loaded by default.

Thus This bit

Screenshot 2021-02-03 at 11 14 16

Could be here ? Screensh![Screenshot 2021-02-03 at 11 14 16](https://user-images.githubusercontent.com/2478303/106739746-629f0d00-6611-11eb-823e-dae58b418448.png)ot 2021-02-03 at 11 14 10

pwalczysko commented 3 years ago

I guess we could immediately show them ALL on the image, but we'd need a way to distinguish those that are actually added to the image (and can be edited) and those that merely could be added from OMERO.

I think so, but probably something for a different PR.. ?

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-figure-and-multiple-rois/48030/5

will-moore commented 3 years ago

@pwalczysko Re: https://github.com/ome/omero-figure/pull/422#issuecomment-772433994 I don't think you'd be disappointed that it hadn't loaded all your ROIs, since you wouldn't know that some were missing until you scroll to the bottom of the list to check that they're not there - and then you'd see "Load more..." ?

To me, it makes sense for a simple "Load more..." to go at the end of the list like:

Whereas for more complex pagination controls it makes sense to put at the top of the list:

Prev page | Next page | Page 1 2 3 Showing Page 1 / 3:

I'll try and see how it looks...

pwalczysko commented 3 years ago

since you wouldn't know that some were missing

You very possibly know your ROIs from iviewer or another viewer, even very probably I would say ? I guess you come to Figure with at least a partially pre-concieved idea about which ROIs to add and why, this would make sense to me...

But as a whole, I definitely do not see the RFE as a blocker, if too difficult to implement or looks bad, then we can leave it.

will-moore commented 3 years ago

Yes, you know your ROIs, but how would you know that they haven't all loaded when you can only see:

Screenshot 2021-02-04 at 22 56 48

You have to scroll down to find your ROIs, then when you get to the bottom you'll see the Load More... button. Moving the controls to the top is certainly a bit more work, since this involves a different UI and a bit of a redesign etc.

pwalczysko commented 3 years ago

Moving the controls to the top is certainly a bit more work, since this involves a different UI and a bit of a redesign etc.

aha, thanks. I think we should leave it then as it is.

will-moore commented 3 years ago

Hmm - Actually, I hate to think of a case where I have thousands of ROIs, I add one more in iviewer, then I want to find it in figure, I'd want to jump to the last page, which could be painful. So I'm just going to do pagination - won't take too long...

will-moore commented 3 years ago

Done. To test:

Screenshot 2021-02-05 at 15 32 41

In the screenshot I temporarily limited the page size to 100 since the image didn't have > 500 ROIs.

pwalczysko commented 3 years ago

Tested the last commit (pagination) with

Worked as expected. Couple of minor RFEs in order of improtance:

  1. Could the dropdown manu with the page list show what page I am corrently on ? This could be done with a tick mark, as is usual in other apps, see screenshot 1 below (which is unfortunately highlighted as 4., cannot do anything about it, sorry).

  2. What is "Export Options" as tooltip on the dropdown menu next to "Next" button ? Does not sound very helpful. Maybe replace with "Select page" see screenshot 2 below.

  3. If there is only one ROI page, the widget with Next and Prev and dropdown should not be shown at all.

  4. Screenshot 2021-02-08 at 11 46 32

  5. Screenshot 2021-02-08 at 11 37 18

will-moore commented 3 years ago

That should address those 3 issues:

pwalczysko commented 3 years ago

That should address those 3 issues:

* show current page on drop-down list

* Fix tooltip (copy and paste error)

* Don't show pagination controls unless needed (> 1 page)

Can confirm the fix, nice behaviour now. Ready to merge fmpov.