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

Spw grid view (Insight) #4874

Merged
merged 33 commits into from Nov 10, 2016
Merged

Spw grid view (Insight) #4874

merged 33 commits into from Nov 10, 2016

Conversation

dominikl
Copy link
Member

@dominikl dominikl commented Sep 30, 2016

What this PR does

Corresponding Insight PR to #4863

  • Always show the well fields view for plates

Limitations:

  • Does not yet support spatial view in the bottom fields view
  • ... and multi selection of fields

Test

See the according Web PR above, make sure Insight basically does the same.

@dominikl
Copy link
Member Author

When the field view is always shown, we could remove the "Field View" button in the tool bar, as well as the "Field view" related code in the DataBrowser, right? The Databrowser is quite complex anyway, would be nice to get rid off some unnecessary code. /cc @jburel
screen shot 2016-09-30 at 11 08 35

@jburel jburel added the develop label Sep 30, 2016
@dominikl
Copy link
Member Author

dominikl commented Oct 4, 2016

Also enabled the old Fields View (see above screenshot) again, so for testing, make sure that also still works.

*/
public WellSampleNode getNode(Point p) {
Component c = findComponentAt(p);
if (c != null && c instanceof FieldDisplay) {
Copy link
Member

Choose a reason for hiding this comment

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

don't need the null check

@pwalczysko
Copy link
Member

Compared to Web, you do not have the Well shown in the RHP when well is clicked.
Instead, it looks always like image is selected, no possibility to select (and annotate) wells.
screen shot 2016-10-06 at 10 07 23

@pwalczysko
Copy link
Member

pwalczysko commented Oct 6, 2016

Compared to Web, I do not have any possiblity to zoom out in the lower panel (where the fields are). This makes the lower panel unusable (by default, the Fields come huge. Note that the control slider in the screenshot regulates the TOP panel only.
screen shot 2016-10-06 at 10 10 07

@pwalczysko
Copy link
Member

What is the point of the dropdown menu (Field #1, Field #2 etc.) when I can easily get into a mismatch like indicated in the screenshot ?
screen shot 2016-10-06 at 10 12 52

@pwalczysko
Copy link
Member

After discussion with @dominikl : the comment #4874 (comment) is touching upon the "relict" feature of Insight, the dropdown menu Field #1 etc. There is this remnant feature with dropdown menu and icon present in Insight which there was not in Web.
I might suggest removing that, but this is quite a decision....
Atm, I think @gusferguson @will-moore @bramalingam and @jburel should have a look at this, whether or not removal of this dropdown is what we want.

The situation is complicated by the Web PR not being in the build (why?), so we cannot even compare.
Maybe the way forward is simply to copy everything exacly as in web, but, see above....

@pwalczysko
Copy link
Member

Stopping testing this until #4874 (comment) is resolved.

@pwalczysko
Copy link
Member

After discussion with @will-moore :

  • The dropdown menu Field#1 Field#2 etc. Spw grid view (Insight) #4874 (comment) is correctly in place, matches with web and makes sense to leave it there.
  • The Web PRs workflow is such that the "Spatial" view will come later, a corresponding PR simply does not exist yet. This would mean that we can say that the "Spatial" button and the non-functional spatial view in Insight might remain as they are, with the outlook to having them fixed as this work progrsses...

@jburel
Copy link
Member

jburel commented Oct 8, 2016

#4874 (comment) sorry for missing comment
I will remove any code/UI that was introduced when the first version was implemented.
No reason to keep it

@jburel
Copy link
Member

jburel commented Oct 8, 2016

Due to the number of fields in some plates (e.g.700) the interaction with menu needs to be carefully tested in both clients @pwalczysko

@pwalczysko
Copy link
Member

Insight handles well 700 fields performance-wise (takes 15-30 sec to upload the 700 fields to the bottom pane.

Web - still waiting for inclusion of the PR into merge.

Re: Insight and 700 fields - it is obvious that the fields are not well labeled, I cannot see the Field number there. We think we should take the approach of Web, having the Well number at the begininng of the row only.

screen shot 2016-10-10 at 14 30 09

@pwalczysko
Copy link
Member

Bug: Select a plate with 6 wells and some 40 fields per well. ID 1805 user-4 on eel merge for example.
Select first well, observe the spinner in the lower pane, then quickly, while the spinner is still spinning, Shift + click on the last well (to multiselect all the wells) - observe error.

java.lang.Exception: Abnormal termination due to an uncaught exception.
java.lang.NullPointerException
    at org.openmicroscopy.shoola.agents.dataBrowser.view.RowFieldCanvas$FieldDisplay.getPreferredSize(RowFieldCanvas.java:283)
    at java.awt.GridBagLayout.GetLayoutInfo(GridBagLayout.java:1115)
    at java.awt.GridBagLayout.getLayoutInfo(GridBagLayout.java:916)
    at java.awt.GridBagLayout.preferredLayoutSize(GridBagLayout.java:736)
    at java.awt.Container.preferredSize(Container.java:1796)
    at java.awt.Container.getPreferredSize(Container.java:1780)
    at javax.swing.JComponent.getPreferredSize(JComponent.java:1664)
    at javax.swing.ScrollPaneLayout.layoutContainer(ScrollPaneLayout.java:791)
    at java.awt.Container.layout(Container.java:1510)
    at java.awt.Container.doLayout(Container.java:1499)
    at java.awt.Container.validateTree(Container.java:1695)
    at java.awt.Container.validateTree(Container.java:1704)
    at java.awt.Container.validateTree(Container.java:1704)
    at java.awt.Container.validate(Container.java:1630)
    at javax.swing.RepaintManager$3.run(RepaintManager.java:711)
    at javax.swing.RepaintManager$3.run(RepaintManager.java:709)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:76)
    at javax.swing.RepaintManager.validateInvalidComponents(RepaintManager.java:708)
    at javax.swing.RepaintManager$ProcessingRunnable.run(RepaintManager.java:1731)
    at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:311)
    at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:756)
    at java.awt.EventQueue.access$500(EventQueue.java:97)
    at java.awt.EventQueue$3.run(EventQueue.java:709)
    at java.awt.EventQueue$3.run(EventQueue.java:703)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:76)
    at java.awt.EventQueue.dispatchEvent(EventQueue.java:726)
    at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:201)
    at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:116)
    at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:105)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:93)
    at java.awt.EventDispatchThread.run(EventDispatchThread.java:82)
Abnormal termination due to an uncaught exception.
java.lang.NullPointerException
    at org.openmicroscopy.shoola.agents.dataBrowser.view.RowFieldCanvas$FieldDisplay.getPreferredSize(RowFieldCanvas.java:283)
    at java.awt.GridBagLayout.GetLayoutInfo(GridBagLayout.java:1115)
    at java.awt.GridBagLayout.getLayoutInfo(GridBagLayout.java:916)
    at java.awt.GridBagLayout.preferredLayoutSize(GridBagLayout.java:736)
    at java.awt.Container.preferredSize(Container.java:1796)
    at java.awt.Container.getPreferredSize(Container.java:1780)
    at javax.swing.JComponent.getPreferredSize(JComponent.java:1664)
    at javax.swing.ScrollPaneLayout.layoutContainer(ScrollPaneLayout.java:791)
    at java.awt.Container.layout(Container.java:1510)
    at java.awt.Container.doLayout(Container.java:1499)
    at java.awt.Container.validateTree(Container.java:1695)
    at java.awt.Container.validateTree(Container.java:1704)
    at java.awt.Container.validateTree(Container.java:1704)
    at java.awt.Container.validate(Container.java:1630)
    at javax.swing.RepaintManager$3.run(RepaintManager.java:711)
    at javax.swing.RepaintManager$3.run(RepaintManager.java:709)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:76)
    at javax.swing.RepaintManager.validateInvalidComponents(RepaintManager.java:708)
    at javax.swing.RepaintManager$ProcessingRunnable.run(RepaintManager.java:1731)
    at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:311)
    at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:756)
    at java.awt.EventQueue.access$500(EventQueue.java:97)
    at java.awt.EventQueue$3.run(EventQueue.java:709)
    at java.awt.EventQueue$3.run(EventQueue.java:703)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:76)
    at java.awt.EventQueue.dispatchEvent(EventQueue.java:726)
    at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:201)
    at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:116)
    at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:105)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:93)
    at java.awt.EventDispatchThread.run(EventDispatchThread.java:82)
Exception in thread "AWT-EventQueue-0"

    at org.openmicroscopy.shoola.env.ui.UserNotifierImpl.showErrorDialog(UserNotifierImpl.java:189)
    at org.openmicroscopy.shoola.env.ui.UserNotifierImpl.notifyError(UserNotifierImpl.java:289)
    at org.openmicroscopy.shoola.env.AbnormalExitHandler.doTermination(AbnormalExitHandler.java:147)
    at org.openmicroscopy.shoola.env.AbnormalExitHandler.terminate(AbnormalExitHandler.java:85)
    at org.openmicroscopy.shoola.env.RootThreadGroup.uncaughtException(RootThreadGroup.java:69)
    at java.awt.EventDispatchThread.processException(EventDispatchThread.java:223)
    at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:215)
    at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:116)
    at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:105)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:93)
    at java.awt.EventDispatchThread.run(EventDispatchThread.java:82)

@pwalczysko
Copy link
Member

RFE: Do not hide the other rows in bottom panel if one field of one row is clicked.

  • have all 6 wells of plate ID 1805 user-4 on eel merge selected
  • observe that this results in having 6 rows of field thumbs in the bottom panel
  • click on a field (e.g. 45) in the last row, marked as D12
  • observe that the rows D7-D11 vanish, only row D12 is visible now -> not expected, and in discrepancy with web

Suggest: adjust the behaviour to web.

@pwalczysko
Copy link
Member

RFE: Show thumbs as they are coming.

  • web is faster in loading the fields into the bottom pane (more than 2x)
  • it also shows the thumbnails gradually appearing in the bottom pane, i.e. you do not have a long wait with just a spinner for company, then all comes at once, instead you see the thumbs slowly popping up one-by-one or two-by-two

Suggest: If the speed cannot be improved, take from web the tactics of showing the thumbs "as they come" which improves the waiting experience remarkably.

@jburel
Copy link
Member

jburel commented Oct 11, 2016

If I remember correctly: the loading strategy at the time of implementation was to load all the fields before display.
I agree with @pwalczysko it should not follow the approach implemented when browsing a plate or a dataset to improve the experience

@will-moore will-moore mentioned this pull request Oct 11, 2016
6 tasks
@pwalczysko
Copy link
Member

Please reverse the large thumbnails for normal datasets by default and also for plates by default as per #4850 (comment).

* @param well
* Pass <code>true</code> if this {@link WellSampleNode}
* represents the well, <code>false</code> if it represents the
* {@link WellSampleData} (ie field)
Copy link
Member

Choose a reason for hiding this comment

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

typo i.e.

WellsModel wm = (WellsModel) model;

List<WellSampleNode> wells = wm.getSelectedWells();
if (wells == null || wells.isEmpty())
Copy link
Member

Choose a reason for hiding this comment

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

CollectionUtils.isEmpty(wells) could be used

if (!(this instanceof WellsModel))
return false;
Point p = new Point(row, column);
List<Point> l = new ArrayList<Point>(1);
Copy link
Member

Choose a reason for hiding this comment

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

List<Point> l = Arrays.asList(p); could be used

view.setFieldMagnificationFactor(v);
else
view.setMagnificationFactor(v);
int value = (int) (v*FACTOR);
Copy link
Member

Choose a reason for hiding this comment

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

maybe introduced a method to reset the value. Same code line 143
to reset the slider value

buildGUI(true);
}

/**
* Check's if the browser currently deals with wells
Copy link
Member

Choose a reason for hiding this comment

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

typo check's


/**
/**
Copy link
Member

Choose a reason for hiding this comment

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

need to keep authors. need to add you details to the list

boolean selectionChanged = false;
Set<Long> ids = new HashSet<Long>();
for (WellSampleNode well : wells) {
if(!well.isWell())
Copy link
Member

Choose a reason for hiding this comment

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

space if

nodes = new ArrayList<WellSampleNode>();
List<String> titles = new ArrayList<String>();
for (WellSampleNode well : wells) {
if(!well.isWell())
Copy link
Member

Choose a reason for hiding this comment

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

same

* @return See above.
*/
WellImageSet getSelectedWell()
WellSampleNode getSelectedWell()
Copy link
Member

Choose a reason for hiding this comment

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

CollectionUtils.isEmpty could be used

if (wis.getRow() == row && wis.getColumn() == column) {
setSelectedWell(wis);
Point targetField = null;
for(Point p : fields) {
Copy link
Member

Choose a reason for hiding this comment

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

spaces

@gusferguson
Copy link

@jburel - 10.0.51.106

@jburel
Copy link
Member

jburel commented Nov 8, 2016

Did you delete the data or create a new user? nothing under root

@gusferguson
Copy link

@jburel - created new user user-1/ome and imported some data.

@jburel
Copy link
Member

jburel commented Nov 8, 2016

Thanks for info

handle = hiBrwView.loadThumbnails(ctx, images,

Collection<DataObject> imgs = new ArrayList<DataObject>();
for(ImageData i : images.values())
Copy link
Member

Choose a reason for hiding this comment

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

space between "for and ("

@jburel
Copy link
Member

jburel commented Nov 8, 2016

minor point: having the spinner next to the loading text might be more obvious but only if easy to lay out

boolean complete = result.size() == images.values().size();

Point well = null;
for(Point p : images.keys()) {
Copy link
Member

Choose a reason for hiding this comment

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

same space for (


EXIT:
for (WellSampleNode w : wells) {
if(w.isWell()) {
Copy link
Member

Choose a reason for hiding this comment

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

space

@@ -32,11 +32,15 @@
import javax.swing.JLabel;
import javax.swing.JPanel;


Copy link
Member

@jburel jburel Nov 8, 2016

Choose a reason for hiding this comment

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

// Third-party etc.
could be removed

@@ -0,0 +1,453 @@
/*
* org.openmicroscopy.shoola.agents.dataBrowser.view.WellFieldsCanvas
Copy link
Member

Choose a reason for hiding this comment

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

remove that line

*/
public WellSampleNode getNode(Point p) {
Component c = findComponentAt(p);
if (c != null && c instanceof FieldDisplay) {
Copy link
Member

Choose a reason for hiding this comment

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

null check not required

@@ -2,10 +2,10 @@
* org.openmicroscopy.shoola.agents.dataBrowser.view.WellFieldsCanvas
*
*------------------------------------------------------------------------------
* Copyright (C) 2006-2009 University of Dundee. All rights reserved.
* Copyright (C) 2006-2016 University of Dundee. All rights reserved.
Copy link
Member

@jburel jburel Nov 8, 2016

Choose a reason for hiding this comment

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

remove line 2

@@ -2,7 +2,7 @@
* org.openmicroscopy.shoola.agents.dataBrowser.view.DataBrowserWellToolBar
*
*------------------------------------------------------------------------------
* Copyright (C) 2006-2008 University of Dundee. All rights reserved.
* Copyright (C) 2006-2016 University of Dundee. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

line 2 should be removed i.e. org.openmicroscopy etc.

@@ -36,13 +38,18 @@
import javax.swing.JPanel;
import javax.swing.JToggleButton;
import javax.swing.JToolBar;
import javax.swing.event.ChangeEvent;
import javax.swing.event.ChangeListener;

//Third-party libraries
import org.jdesktop.swingx.JXBusyLabel;
Copy link
Member

Choose a reason for hiding this comment

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

remove //third-party etc

@@ -2,7 +2,7 @@
* org.openmicroscopy.shoola.agents.dataBrowser.view.PlateGridUI
*
*------------------------------------------------------------------------------
* Copyright (C) 2006-2010 University of Dundee. All rights reserved.
* Copyright (C) 2006-2016 University of Dundee. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

remove line 2

//Third-party libraries
import info.clearthought.layout.TableLayout;


import org.apache.commons.collections.CollectionUtils;
Copy link
Member

Choose a reason for hiding this comment

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

remove //third-party and co.

@@ -1379,6 +1379,12 @@ public void setSelectedNodes(Object nodes)
if (selection == null || selection.size() == 0) return;
Copy link
Member

Choose a reason for hiding this comment

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

Could also be replaced by CollectionUtils.isempty

@jburel
Copy link
Member

jburel commented Nov 8, 2016

few comments on code

@jburel
Copy link
Member

jburel commented Nov 9, 2016

@dominikl: Thanks.

@dominikl
Copy link
Member Author

dominikl commented Nov 9, 2016

I'll also move the "busy" icon next to the "Loading..." label, should be easy to change the layout.

@dominikl
Copy link
Member Author

dominikl commented Nov 9, 2016

Review: Just check the layout (text and busy icon) of the metadata DummyPanel (shown while the "Preview" is loading or a well is selected when "Preview" tab is active).

@dominikl dominikl closed this Nov 9, 2016
@dominikl dominikl reopened this Nov 9, 2016
@gusferguson
Copy link

@dominikl

Tested with OMERO.insight-5.3.0-m4-891-929ae02-ice36-b484-win eel user-3.

Loading with spinner and Preview not available text showing as expected in Preview tab of RHP.
Good to merge.

@jburel
Copy link
Member

jburel commented Nov 10, 2016

Thanks Merging

@jburel jburel merged commit 8e460b9 into ome:develop Nov 10, 2016
@jburel jburel added this to the 5.3.0 milestone Mar 29, 2017
@dominikl dominikl deleted the spw_grid_view branch May 3, 2017 09:26
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.

None yet

6 participants