ome / omero-parade

OMERO.web plugin for displaying Dataset thumbnails or Plates in webclient center panel
GNU Affero General Public License v3.0
1 stars 12 forks source link

Expand all for Screens #39

Open chris-allan opened 6 years ago

chris-allan commented 6 years ago

Loads the metadata for all relevant metadata when an SPW container is selected.

All of the AJAX calls have been converted to Axios, follow an asynchronous paradigm, are cancellable, and where appropriate now have loading spinners.

/cc @emilroz

chris-allan commented 6 years ago

First pass.

New Axios data loading infrastructure now applies to all data types. You should definitely have a nicer, cleaner and more visible status when working with PDI. I'll be working on adjusting the filter and data loading stack to support Screens now.

/cc @emilroz

will-moore commented 6 years ago

In Layout.js, this.props.fieldId is now undefined, so loading right panel on Well click fails...

    setSelectedWells(wellIds) {
        this.setState({selectedWellIds: wellIds});
        // Trigger loading Wells in right panel...
        var well_index = this.props.fieldId;
        var selected_objs = wellIds.map(wId => ({id: 'well-' + wId, index: this.props.fieldId}))
...

since we get url like /webclient/metadata_details/well/178/ without ?index=0 which gives an error. Clearly, webclient should handle a missing index a bit better, but in the meantime we need an index. Also the line var well_index = this.props.fieldId; is redundant (probably was before too).

will-moore commented 6 years ago

Testing locally with development server, I see some slightly strange sequence when clicking on a Screen (with 4 Plates) which already viewing a Screen (with 2 Plates, displayed in centre):

Basically, the selection change event in the tree triggers re-loading of thumbnails for the 2 Plates already displayed (even though a different Screen has just been selected) so I see thumbnail spinners and thumbnails loading right before the old 2 Plates are replaced by the 4 new Plates (when those AJAX calls return)

screen shot 2018-05-15 at 11 08 05

Sequence is hard to read as URLs are not shown, but after /dataproviders/ url are 4 calls for fields for new Plates, then the 2 thumbnail requests for the old Plates, then the 4 webgateway/plate/id/0/ calls for the new Plates and finally the 4 thumbnail calls for those Plates.

I'll maybe hold off more testing for now until deployed on web-dev-merge.

chris-allan commented 6 years ago

There is definitely some weird stuff happening when you switch back and forth. Especially when running on a server where there is a single thread, Chrome is making 6 requests in parallel, and a lot of cancellation needs to happen.

Let me finish up making the filtering and data loading Screen aware and I'll switch back to looking at that.

chris-allan commented 6 years ago

Should now be much more solid and ready for tomorrow's testing on web-dev-merge.

will-moore commented 6 years ago

Everything (apart from the thumb slider) seems to be working nicely. I don't see the re-loading of thumbnails right before an old plate is replaced with a new one as reported above. Clicking a Well to load in right panel is also fixed.

will-moore commented 6 years ago

Testing the heatmaps on Dataset, it took me a while to see the SETTINGS control at the bottom. I only found it on the UI after first reading the code to see that it's in the footer. I think it needs to be at the top somewhere otherwise it will be missed. Possibly at the right of the "Add Table Data" row?

I'm not sure what the "Normal" option is for? Not loading thumbnails or heatmap. Is this to turn thumbnails off so we don't load too many for big plates? Maybe the thumbnails placeholders should be black (or darker?). It somehow seems wrong with grey, like we're waiting for them to load?

screen shot 2018-05-16 at 22 56 20

It sounds like "Normal" should be the default setting. But actually "Thumbnails" is the default, right? Maybe you don't need a "Normal" option, you can just toggle off the "Thumbnails" option? (and replace Thumbnails with Heatmap if that is chosen)?

On the List view, the SETTINGS menu doesn't work, but I'm still not seeing thumbnails (no way to turn them back on in this view)

It feels like the HeatMap options should allow me to choose from the same list as "Add Table data..." dropdown (to choose from options that I haven't yet loaded, but would be loaded when I request them).

I'm trying to think of how to have heat-maps not behave so differently in List view vv Thumbnails view. It seems wrong that I can choose a column in the List view and check the checkbox to show Heatmap, but when I go to Thumbnails view I don't see that heatmap - then I go back to List view and the Heatmap has gone. The key problem is that List view allows multiple Heatmaps but Thumbnails view only ONE. We could limit the List view to only allow a single Heatmap, but I think it is useful to be able to compare heatmaps from multiple columns. Needs some more thought...

I like the numbers in the thumbnails for heatmap, although this also needs formatting to 2 decimal places for floats (not necessarily in this PR).

will-moore commented 6 years ago

Minor point: The spinners while loading Filters or Table data are shown to the left of the Filter label and "Add Table data..." chooser, so when the spinner is shown/hidden, these elements jump to the side. It's more noticeable for the "Add Table data..." chooser than for the filters. Showing the spinner to the right of this would make it a little smoother.

will-moore commented 6 years ago

Not related to this PR (except wrt trying to improve UI clarity): The "Add table data..." label and the "Table_* " options within it both use the term "Table" to refer to different things (parade table/list view vv OMERO.Tables), which is confusing. Also, you now have to use "Add table data..." for heatmap, even if you never display this data in a table. So maybe this should just be "Load data..." ?

chris-allan commented 6 years ago

Based on the feedback above:

Also, when loading data from the server I had a subtle bug, which is now fixed, where I was not specifying the field when doing Screen --> Plate --> Well --> WellSample --> Image queries. This resulted in potentially the wrong Image ID being put in the returned data and subsequent holes in the heatmaps.

We will have to be very careful with all of this going foward when we start bringing selectable fields into Parade. Especially if the field counts of Plates that are in a Screen differ.

I also had a lapse in logic where asynchronous calls for all thumbnails were being queued in the browser. On some of our test cases this could easily be >100. Exactly what I was previously afraid would happen did, promises stacked up and performance really suffered. The number of thumbnail batches that can load at one time is now limited to 6.

will-moore commented 6 years ago

As discussed - there is probably too much here to try and get in to the first 0.0.1 release, so we'll exclude so we can test the release candidate build.

Note: J-M removed the exclude on 25/07

jburel commented 6 years ago

First round:

jburel commented 5 years ago

@chris-allan what is the status of this PR?

53 is conflicting with it. #53 is needed to fix a build failure

chris-allan commented 5 years ago

@jburel: I haven't had time to get back to this fully based on the strategy of removing some of the more technically dangerous conditions that we discussed a while back. The contents of #53 are a bit troublesome with how much of the package lock file has changed.

Let's get together some time today to go over some options.

jburel commented 5 years ago

Discussed this am with @chris-allan. The plan is:

snoopycrimecop commented 5 years ago

Conflicting PR. Removed from build OMERO-plugins-push#26. See the console output for more details. Possible conflicts:

--conflicts

jburel commented 5 years ago

--exclude