Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rois pagination #422

Merged
merged 9 commits into from
Feb 19, 2021
Merged

Rois pagination #422

merged 9 commits into from
Feb 19, 2021

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Jan 29, 2021

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...

  • Find an image that has lots of ROIs - (over 500)
  • Open the Edit ROIs dialog and load ROIs from OMERO
  • The first page of 500 ROIs should load (as before) but if you scroll to the bottom, there is some info on how many you've loaded and a "Load More..." button.
  • Clicking the button should load more. When there are no more to load, the button should be hidden. ROIs should be added when you click Add button for a ROI.

Screenshot 2021-01-31 at 23 43 51

To test Crop dialog...

  • Find an image that has lots of ROIs - (over 200) including Rectangles
  • Open the Crop dialog. The first 200 ROIs are loaded and any Rectangles are shown as thumbnails for picking a crop region (as before).
  • Scrolling to the bottom shows info on how many ROIs loaded and a Load More... button. This should load more and the be hidden when no more to load.

Screenshot 2021-01-30 at 22 53 16

@pwalczysko
Copy link
Member

pwalczysko commented Feb 2, 2021

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
Copy link
Member Author

Thanks:

    1. We don't have any way to load ONLY Rectangle ROIs from OMERO using the JSON API (which is what I'm currently using). I could write a custom 'Rectangle loader' for figure, which would provide a nicer experience and might not be very much code. I'll look into it...
    1. I originally was going to add a preview for every Shape in the Edit ROIs dialog, but this is a lot of work to support previews for all types of Shape. Also, it would potentially load a lot of pixel data to show these. 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.

@will-moore
Copy link
Member Author

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

@pwalczysko
Copy link
Member

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 ?
ScreenshScreenshot 2021-02-03 at 11 14 16ot 2021-02-03 at 11 14 10

@pwalczysko
Copy link
Member

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
Copy link

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
Copy link
Member Author

@pwalczysko Re: #422 (comment)
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:

  • item
  • item
  • item
  • Load more...

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:

  • item
  • item
  • item

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

@pwalczysko
Copy link
Member

pwalczysko commented Feb 4, 2021

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

Done. To test:

  • Try with image that has fewer than 500 ROIs, more than 500 ROIs and none:

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
Copy link
Member

pwalczysko commented Feb 8, 2021

Tested the last commit (pagination) with

  • more than 2000 ROIs images
  • 3 and 20 ROIs images
  • no ROI image

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.

Screenshot 2021-02-08 at 11 46 32

Screenshot 2021-02-08 at 11 37 18

@will-moore
Copy link
Member Author

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)

@pwalczysko
Copy link
Member

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.

@jburel jburel merged commit 3854cd9 into ome:master Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle >1k ROIs
4 participants