Expand all for Projects #20

chris-allan · 2018-04-11T12:35:53Z

Add the capability of opening all Datasets in a Project if they are all closed
and change the semantics so that expanding or collapsing a node,
not selecting it, affects what is shown by Parade.

In order to provide functionality to open all Dataset children for a
selected Project the component hierarchy needs to reflect what nodes
of the jsTree that are open rather than just those that are
selected. This PR contains refactoring to all jsTree node handling,
initial state, and resulting subcomponent usage so as to provide the
display components with reliable and thorough context under which
to operate.

The stage is further set to enable the same type of logic for Screens
once we are ready.

Note that the changes in this PR bind Parade to the base level
selection_change.ome event.

In order to provide functionality to open all Dataset children for a selected Project the component hierarchy needs to reflect what nodes of the jsTree that are *open* rather than just those that are *selected*. It is easiest to do this via component key at the right level which will affect mount/unmount status and this is exactly what this commit does. The stage is further set to enable the same type of logic for Screens once we are ready.

chris-allan · 2018-04-11T12:42:27Z

Some investigation into jsTree event batching, delayed asynchronous data loading, and/or axios usage will need to be done. Due to the mount/unmount churn that occurs when many Datasets are opened programmatically, it is possible to reproduce this by doing the same quickly by hand, setState() is being called on unmounted components from AJAX call result handlers.

Some related reading:

https://reactjs.org/blog/2015/12/16/ismounted-antipattern.html

snoopycrimecop · 2018-04-15T23:09:29Z

Conflicting PR. Removed from build parade-merge#40. See the console output for more details.
Possible conflicts:

PR Sparklines #19 chris-allan 'Sparklines'
- omero_parade/static/omero_parade/css/parade.css

--conflicts

Based on the previous work to enable opening all Datasets under a given Project this refactors the parade entrypoint quite substantially. App and DataContainer are now free to bind event handling themselves. DataContainer state now reflects the set of selected, root, effective root, and open jsTree nodes. The "effective root" reflects the current intended semantics surrounding the conditions where multiple selection is acceptable. It also reflects the underlying user interface assumptions surrounding cross root node, mostly Project and Screen, selection and data display. Should we wish to relax any of these aforementioned assumptions, to for example allow parade to display data from *multiple* Projects at one time, there is now one place to change. The stage is further set for the impending changes which will allow parade to display multiple Plates from a single Screen.

chris-allan · 2018-04-16T12:53:30Z

Large refactor done in 66c6879. Probably at least one more round to come.

/cc @emilroz

chris-allan · 2018-04-16T18:03:56Z

That's everything pushed down as far as the jsTree nodes are used.

will-moore · 2018-04-16T22:35:36Z

src/js/dataLoader/DataContainer.js

@@ -47,51 +212,59 @@ class DataContainer extends React.Component {
 }

 render() {
- var parentNode = this.props.parentNode;
+ const jstree = this.props.jstree;
+ const effectiveRootNode = this.state.effectiveRootNode;


On the first render here when you first switch to the Parade app, the effectiveRootNode is undefined because the app has not received any events, and it's not passed in the current selected node initially as previously:

var tree_selected = datatree.get_selected(true);

However, I don't see any easy fix since you can't get the selected nodes and setState() with them in the constructor (not allowed to setState() when not mounted).

The constructor receives the current jsTree instance as a constructor argument so yes it can get the selected nodes, and consequently also all the other dependent state. It can then use all of it as initial state.

This has been done in 8d1fccc.

snoopycrimecop · 2018-04-16T23:09:29Z

Conflicting PR. Removed from build parade-merge#41. See the console output for more details.
Possible conflicts:

PR Sparklines #19 chris-allan 'Sparklines'
- omero_parade/static/omero_parade/css/parade.css

~~--conflicts~~ Conflict resolved in build parade-merge#42. See the console output for more details.

chris-allan · 2018-04-17T07:34:18Z

Some related reading I came across during investigation for the refactoring completed in this PR :

In reading several StackOverflow and blog posts it seems like the consensus is that the use of component key changes to force remounting is treated as an anti-pattern. More recently there has also been a push within the React ecosystem towards refactoring the component lifecycle:

https://reactjs.org/blog/2018/03/29/react-v-16-3.html#component-lifecycle-changes

In order to protect ourselves moving forward we might consider reviewing all our use of lifecycle methods and upgrading to 16.3.x before minting a 0.1.0 release.

/cc @jburel

will-moore · 2018-04-17T20:45:22Z

Looks good, and is working fine.
Good to merge.

chris-allan · 2018-04-18T07:40:10Z

Current implementation results in a much higher number of FilterHub.render() calls resulting in sluggish performance in a variety of scenarios including selection. Need to look into this before we merge.

/cc @emilroz

As `render()` may be called many times, recalculating the data provided to subcomponents every time can be quite costly. Especially for larger numbers of images. This image JSON is now stored in the component state and only modified when the set of open or selected nodes changes.

As `render()` may be called many times, recalculating the filtered set of image JSON to provide to subcomponents every time can be quite costly. Especially for larger numbers of images. This filtered image JSON is now stored in the component state and only modified when a filter state changes.

If we listen to "select_node.jstree" or "changed.jstree" we will both be smashing the jsTree with events and be smashed by the jsTree by events. All of which we would end up having to reconcile in our React components to accurately reflect legitimate change. The existing thumbnail plugin works around this issue by disabling jsTree event propogation when making large selections and then by making use of the "selection_change.ome" event. As the jsTree accurately bubbles "selection_change.ome" events up for the purposes of several other components we can also listen to these exclusively for our own purposes.

chris-allan · 2018-04-18T14:48:44Z

Aforementioned concerns addressed. Ready for review again. Pay particular attention to performance when selecting >20 images by drag selection in the centre panel.

/cc @emilroz

will-moore · 2018-04-18T20:54:26Z

Certainly rendering speed has improved with those last few commits.
Just to be clear - there's no new functionality in this PR (from the user's point of view)?
Current functionality is working well and code looks good.
Good to merge for me.

chris-allan · 2018-04-18T20:57:38Z

The big user interface addition here is the capability of opening all Datasets in a Project if they are all closed and that expanding or collapsing a node, not selecting it, affects what is shown by Parade. Everything else is to facilitate those changes.

will-moore · 2018-04-18T22:06:39Z

Ah, OK - Don't know how I missed that.
Seems to be working fine :)

chris-allan · 2018-04-19T07:11:55Z

👍

Updated the PR description to be a little more prescriptive about all of that.

chris-allan · 2018-04-19T08:44:27Z

@emilroz came up with a little script to generate hierarchies for testing:

https://gist.github.com/emilroz/9956daab41405b8b27b0ef1510893160

With 50 Datasets containing 20 Images each the open_node.jstree event churn and subsequent repeated translation of the node hierarchy to JSON is almost unbearable even on my very powerful system. I'll add something to help with that here in a second.

/cc @emilroz, @jburel

chris-allan · 2018-04-19T08:49:28Z

d7c2582 does a nice job of reducing the event churn. There should probably be a progress indicator after "Open All" is clicked as opening and loading 50 Datasets is definitely not instant but that can come later. There are probably lots of cases where we should do that.

If @emilroz is happy with the performance now we can get this merged and re-focus our attention on thumbnails.

emilroz · 2018-04-19T10:54:00Z

All changes look good to me.

chris-allan added 2 commits April 11, 2018 10:17

Keep entire UI out of the way of overhanging controls

2510e28

Push logic one level down

7047afa

chris-allan requested a review from will-moore April 16, 2018 18:04

Clean up unused code

1186ca6

will-moore reviewed Apr 16, 2018

View reviewed changes

Set initial state in constructor

8d1fccc

chris-allan mentioned this pull request Apr 17, 2018

Sparklines #19

Merged

Merge remote-tracking branch 'origin/master' into expand-all

bbc5113

chris-allan requested a review from will-moore April 17, 2018 09:28

chris-allan added 5 commits April 18, 2018 10:36

Fix fileset selection

6603316

If function loading fails, return early

900aafd

Ignore incremental events when opening all

d7c2582

emilroz approved these changes Apr 19, 2018

View reviewed changes

jburel merged commit 6b41832 into ome:master Apr 19, 2018

chris-allan mentioned this pull request May 1, 2018

OMERO.web centre panel cleanup ome/openmicroscopy#5746

Merged

jburel added this to the 0.1.0 milestone May 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand all for Projects #20

Expand all for Projects #20

chris-allan commented Apr 11, 2018 •

edited

Loading

chris-allan commented Apr 11, 2018

snoopycrimecop commented Apr 15, 2018

chris-allan commented Apr 16, 2018

chris-allan commented Apr 16, 2018

will-moore Apr 16, 2018

will-moore Apr 16, 2018

chris-allan Apr 17, 2018 •

edited

Loading

snoopycrimecop commented Apr 16, 2018 •

edited

Loading

chris-allan commented Apr 17, 2018

will-moore commented Apr 17, 2018

chris-allan commented Apr 18, 2018

chris-allan commented Apr 18, 2018

will-moore commented Apr 18, 2018

chris-allan commented Apr 18, 2018

will-moore commented Apr 18, 2018

chris-allan commented Apr 19, 2018

chris-allan commented Apr 19, 2018

chris-allan commented Apr 19, 2018

emilroz commented Apr 19, 2018

Expand all for Projects #20

Expand all for Projects #20

Conversation

chris-allan commented Apr 11, 2018 • edited Loading

chris-allan commented Apr 11, 2018

snoopycrimecop commented Apr 15, 2018

chris-allan commented Apr 16, 2018

chris-allan commented Apr 16, 2018

will-moore Apr 16, 2018

Choose a reason for hiding this comment

will-moore Apr 16, 2018

Choose a reason for hiding this comment

chris-allan Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

snoopycrimecop commented Apr 16, 2018 • edited Loading

chris-allan commented Apr 17, 2018

will-moore commented Apr 17, 2018

chris-allan commented Apr 18, 2018

chris-allan commented Apr 18, 2018

will-moore commented Apr 18, 2018

chris-allan commented Apr 18, 2018

will-moore commented Apr 18, 2018

chris-allan commented Apr 19, 2018

chris-allan commented Apr 19, 2018

chris-allan commented Apr 19, 2018

emilroz commented Apr 19, 2018

chris-allan commented Apr 11, 2018 •

edited

Loading

chris-allan Apr 17, 2018 •

edited

Loading

snoopycrimecop commented Apr 16, 2018 •

edited

Loading