Large selection #2469

jburel · 2014-05-08T14:07:23Z

Review selection and loading of annotations.
Few methods were loaded twice and call not optimised.
see https://trac.openmicroscopy.org.uk/ome/ticket/12086

To test:

Log in as user-4
Display read-only group
Expand the Orphaned images folder (>800 images)
Select all in the images (~800) from the tree
Deselect
Select now from the central panel.

The method was invoked twice. This means that the annotation were loaded twice.

When selected the node (displayed in tree), the annotations were loaded twice.

This could lead to out of memory exception due to the number of call.

bpindelski · 2014-05-09T09:24:14Z

This still causes issues. Tested on Ubuntu. Followed these steps:

Log in as user-4
Display read-only group
Expand the Orphaned images folder (>800 images)
Select all in the images (~800) from the tree (holding the Shift key and clicking)
Deselect by clicking on a single element in the tree
Select from the central panel (again using Shift)
Deselect in the central panel by clicking on a single item in the central panel

Insight hangs. I attached a jstack output to the ticket.

jburel · 2014-05-09T10:26:59Z

@bpindelski:

Did you wait for the annotations to be loaded for the first selection (800), before switching to another selection?

bpindelski · 2014-05-09T10:58:45Z

Yes, I tested again and waited a minute before doing each select/deselect. From what I see, the hang happens when I deselect a large selection by using the central panel - deselecting in the tree doesn't upset Insight.

jburel · 2014-05-09T10:59:45Z

@bpindelski: thanks for info

Issue occurred when deselecting from central panel using the table view.

jburel · 2014-05-09T12:31:19Z

@bpindelski: The last commit should fix the issue.

bpindelski · 2014-05-09T13:26:41Z

components/insight/SRC/org/openmicroscopy/shoola/agents/dataBrowser/browser/BrowserModel.java

-			removeSelectedDisplay(previouslySelectedDisplay);
+		Colors colors = Colors.getInstance();
+		for (final ImageDisplay node : previouslySelectedDisplays) {
+	        node.setHighlight(colors.getDeselectedHighLight(node));


Missing indentation.

bpindelski · 2014-05-09T13:33:47Z

From the UX perspective - this is good to merge. From the code perspective - I'd rather ask someone more knowledgeable in Insight code to review the changes. @dominikl or @mtbc maybe?

mtbc · 2014-05-09T13:39:20Z

I did actually glance it over previously but, apart from the minor thought that the Map<Long, List<IObject>> could have been a Multimap<Long, IObject> for convenience, certainly didn't see any obvious problems. There are a lot of lines changed though so I would encourage @dominikl to take a look too.

jburel · 2014-05-09T13:48:10Z

@bpindelski: unless I totally reformat the code, we will have the issue with the indentation. (i.e. tabs vs space). I can do it when the review is complete otherwise it is very difficult to review it.

bpindelski · 2014-05-09T13:52:12Z

@jburel The indentation is a "nice-to-have", but not a blocker.

jburel · 2014-05-09T13:54:18Z

I would like to clean it, just due to the number of changes I was holding off. Will do it when we are all happy with the changes.

dominikl · 2014-05-09T15:22:55Z

Looks good to me, too.

joshmoore · 2014-05-14T06:38:31Z

Next step?

jburel · 2014-05-14T07:56:50Z

I will push the multimap commit later today then we should be ready.

jburel · 2014-05-14T14:24:16Z

@mtbc: replace map by multimap. no more changes to come.

mtbc · 2014-05-15T07:40:41Z

...onents/insight/SRC/org/openmicroscopy/shoola/agents/treeviewer/view/TreeViewerComponent.java

+            } catch (Exception e) {
+                UserNotifier un =
+                        TreeViewerAgent.getRegistry().getUserNotifier();
+                un.notifyInfo("Image Not valid", 


here and below, weird to capitalize "not" but not "valid"

mtbc · 2014-05-15T07:50:05Z

Seems to actually work just fine, no actual blockers to merging.

mtbc · 2014-05-15T07:51:58Z

components/insight/SRC/org/openmicroscopy/shoola/env/data/OmeroMetadataServiceImpl.java

 			}
 		}
+		Map<Long, Collection<AnnotationData>> filesetMap =
+		        new HashMap<Long, Collection<AnnotationData>>();


maybe move the possibly wasted new to an else, can make the variable final to make sure it does get assigned

mtbc · 2014-05-15T08:03:41Z

For the avoidance of doubt, this is already okay to merge, it's a clear improvement that should be in 5.0.2.

jburel · 2014-05-15T21:15:41Z

@mtbc: as indicated previously, I won't fix indent this makes the changes difficult to review.
This is due to a mix of tabs and spaces usage now. Happy to do it but we must be ready for a 90% "rewrite" of each class.

I can do that when we are happy with the changes

mtbc · 2014-05-16T08:35:06Z

I tried to comment only on indent that was within a larger block of green added lines. Still, latest commits look good, this PR remains good to merge.

joshmoore · 2014-05-16T13:37:11Z

Thanks, guys.

Large selection

jburel · 2014-05-19T08:24:54Z

--rebased-to #2512

jburel added 9 commits May 7, 2014 11:33

Fix message.

adb57d9

Set the related nodes only once.

eb88fa8

The method was invoked twice. This means that the annotation were loaded twice.

Refactor code to reduced the number of calls.

63b0d2f

Refactor code to limit number of calls.

9db063a

Add doc.

92b4c9b

Load the annotation only once.

e172d73

When selected the node (displayed in tree), the annotations were loaded twice.

Load only once the annotation.

84be771

Fire property only once.

4b08c53

This could lead to out of memory exception due to the number of call.

Remove System.err and update header.

ed542af

jburel added the dev_5_0 label May 8, 2014

Only load data once when deselecting from central panel

6419a1c

Issue occurred when deselecting from central panel using the table view.

bpindelski reviewed May 9, 2014
View reviewed changes

Use multimaps instead of map

a3dc15f

Add missing javadoc.

5e48222

mtbc reviewed May 15, 2014
View reviewed changes

jburel added 4 commits May 15, 2014 22:18

Capitalize.

dbfc147

Move imports.

127003c

Capitalize

1b6aeaf

Check emptiness.

645ba74

joshmoore added a commit that referenced this pull request May 16, 2014

Merge pull request #2469 from jburel/large-selection

58b5cd7

Large selection

joshmoore merged commit 58b5cd7 into ome:dev_5_0 May 16, 2014

jburel deleted the large-selection branch May 19, 2014 08:22

jburel mentioned this pull request May 19, 2014

Large selection develop #2512

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Large selection #2469

Large selection #2469

jburel commented May 8, 2014

bpindelski commented May 9, 2014

jburel commented May 9, 2014

bpindelski commented May 9, 2014

jburel commented May 9, 2014

jburel commented May 9, 2014

bpindelski May 9, 2014

bpindelski commented May 9, 2014

mtbc commented May 9, 2014

jburel commented May 9, 2014

bpindelski commented May 9, 2014

jburel commented May 9, 2014

dominikl commented May 9, 2014

joshmoore commented May 14, 2014

jburel commented May 14, 2014

jburel commented May 14, 2014

mtbc May 15, 2014

mtbc commented May 15, 2014

mtbc May 15, 2014

mtbc commented May 15, 2014

jburel commented May 15, 2014

mtbc commented May 16, 2014

joshmoore commented May 16, 2014

jburel commented May 19, 2014

Large selection #2469

Large selection #2469

Conversation

jburel commented May 8, 2014

bpindelski commented May 9, 2014

jburel commented May 9, 2014

bpindelski commented May 9, 2014

jburel commented May 9, 2014

jburel commented May 9, 2014

bpindelski May 9, 2014

Choose a reason for hiding this comment

bpindelski commented May 9, 2014

mtbc commented May 9, 2014

jburel commented May 9, 2014

bpindelski commented May 9, 2014

jburel commented May 9, 2014

dominikl commented May 9, 2014

joshmoore commented May 14, 2014

jburel commented May 14, 2014

jburel commented May 14, 2014

mtbc May 15, 2014

Choose a reason for hiding this comment

mtbc commented May 15, 2014

mtbc May 15, 2014

Choose a reason for hiding this comment

mtbc commented May 15, 2014

jburel commented May 15, 2014

mtbc commented May 16, 2014

joshmoore commented May 16, 2014

jburel commented May 19, 2014