Skip to content

Roifolders improvements#4549

Merged
sbesson merged 9 commits intoome:regionsfrom
dominikl:roifolders_improvements
Apr 4, 2016
Merged

Roifolders improvements#4549
sbesson merged 9 commits intoome:regionsfrom
dominikl:roifolders_improvements

Conversation

@dominikl
Copy link
Copy Markdown
Member

This PR...

  • enables the saving of imageless ROIs; Test: Just check integration test (testSaveImagelessROIs); it doesn't have a real significance in the client, yet.
  • adds some performance tweaks to the ROITable/ObjectManager; Test: Check that the ROI/Folder table performs well with images with many ROIs. For example each ROI/Folder action (e. g. add ROI to folder, via menu or DnD, doesn't matter) as well as putting folders on display via the filter menu, refreshes the table. The refresh lag should not be too long, so that you're still able to interact with the table in a reasonable way.
  • fixes the bug that it was not possible to deselect all folders for display (filter popup menu). Test: Select some folders for display, successively deselect them again (not via the 'Select All' menu item). It should be possible to also deselect the last one, with the result that no folders are on display at all.

IMHO even the interactions with the 1000 ROIs fake image work pretty well. Is that already close to a "real world" example? Looking forward to test the ROI tool with an IDR example...

@jburel jburel added the regions label Mar 30, 2016

/** Rebuilds Tree */
void rebuildTable() {
void rebuildTable() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need to add trailing space

@dominikl dominikl mentioned this pull request Mar 31, 2016
@pwalczysko
Copy link
Copy Markdown
Member

The server on regions is down atm after beluga restart. Will test after the server is back.

@pwalczysko
Copy link
Copy Markdown
Member

Opened the image with 1000 ROIs as user-14 on the regions server. The performance was good (display of the ROIs and display of the MT table).
But then I added 390 of the ROIs into a newly created folder. This took almost a minute to finish....

@pwalczysko
Copy link
Copy Markdown
Member

Another performance issue - after I have the 390 ROIs out of 1000 in a folder, I am making the folder visible and invisible. The action goes ahead quickly. But if I repeat the visible-invisible-visible, then I see some lag, although not always.

@pwalczysko
Copy link
Copy Markdown
Member

Took another 200 ROIs and tried to put them into a new folder. Hit the

java.lang.Exception: java.lang.NullPointerException
    at org.jdesktop.swingx.treetable.AbstractMutableTreeTableNode.remove(AbstractMutableTreeTableNode.java:114)
    at org.openmicroscopy.shoola.agents.measurement.util.roitable.ROINode.remove(ROINode.java:343)
    at org.openmicroscopy.shoola.agents.measurement.view.ROITable.removeROIShape(ROITable.java:713)

already reported by Gus

@pwalczysko
Copy link
Copy Markdown
Member

@sbesson : According to @dominikl , the integration tests are run by the build and the build fails if they do not pass - can you please confirm this assumption ?

@dominikl
Copy link
Copy Markdown
Member Author

dominikl commented Apr 1, 2016

Fixed the NullPointerException. Can confirm that adding many ROIs to the a folder takes quite some time. That's a serverside problem. Not much I can do on the client side. Which raises the question, if we would need a better indication, that a "Save" action is still ongoing. Dialog? Progressbar (can't show the real progress however)? You can see on the status label at the bottom left, that a Save action is ongoing, but that's not really eye catching.

@mtbc
Copy link
Copy Markdown
Member

mtbc commented Apr 1, 2016

You're adding them in batches at least rather than one-by-one?

@pwalczysko
Copy link
Copy Markdown
Member

if we would need a better indication, that a "Save" action is still ongoing. Dialog? Progressbar ....

I think it would be better to make it faster.... as for the indication, why not to pop up Activities ? This is an obvious place to report ongoing actions in Insight.

@dominikl
Copy link
Copy Markdown
Member Author

dominikl commented Apr 1, 2016

Yes, it's done in batches. But it might not be a big problem anyway, if the user has to wait for a bit. I don't think shifting batches of hundreds of ROIs around is a task the user wants to perform with a frequency of multiple times per minute, is it? Just a good indication that the action is still ongoing, might be worth implementing.

@dominikl
Copy link
Copy Markdown
Member Author

dominikl commented Apr 1, 2016

@pwalczysko Ah, the 'Activities' is a good idea, thanks. I'll look into that. But that'd be a separate PR.

@pwalczysko
Copy link
Copy Markdown
Member

shifting batches of hundreds of ROIs.....

As discussed with @dominikl , putting hundred of ROIs into one folder in order to make manageable the lets say 300 ROIs I have in the image is a pretty obvious task (partition the sea of ROIs in you image into smaller, manageable batches, which you can later make visible/invisible)

@dominikl dominikl force-pushed the roifolders_improvements branch from b1045f6 to cdd44ab Compare April 1, 2016 10:11
@dominikl
Copy link
Copy Markdown
Member Author

dominikl commented Apr 1, 2016

Re-review: Last two commits only.

  • Add some ROIs to a folder, add some other ROIs to another folder (in the same 'session', i.e. without closing/reopening the ROI tool in between); this previously triggered the NullPointerException
  • Check that you can deselect all folders from display (folder filter popup menu) (see last point of the updated PR description)
  • Check integration tests, in particular testSaveImagelessROIs (might already be sufficient if the build didn't fail, @sbesson )

(Performance issues and/or usage of Activities dialog have to fixed in separate PR)

@mtbc
Copy link
Copy Markdown
Member

mtbc commented Apr 4, 2016

@dominikl
Copy link
Copy Markdown
Member Author

dominikl commented Apr 4, 2016

Thanks @mtbc , didn't know where to find the test results.

@pwalczysko
Copy link
Copy Markdown
Member

Both commits due to test were fine

  • no NPE hit during the workflow indicated (with existing and new folders)
  • the folders can be consecutively selected and deselected up to having again no folders on display at all

Ready to merge.

@dominikl
Copy link
Copy Markdown
Member Author

dominikl commented Apr 4, 2016

Thanks @pwalczysko . Will investigate/tackle the performance problem of the "add ROIs to folder" action in a separate PR.

@sbesson sbesson merged commit 6f6142b into ome:regions Apr 4, 2016
import java.util.concurrent.ExecutionException;

import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.lang.NotImplementedException;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: Use of this seems new to me. Not that it's a bad idea, but we may need to make sure it's use is documented, etc.

@dominikl dominikl deleted the roifolders_improvements branch April 15, 2016 09:35
@sbesson sbesson added this to the 5.3.0 milestone Apr 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants