linnarsson-lab / loom-viewer

Tool for sharing, browsing and visualizing single-cell data stored in the Loom file format
BSD 2-Clause "Simplified" License
35 stars 6 forks source link

Reorganise view settings in redux #51

Closed JobLeonard closed 8 years ago

JobLeonard commented 8 years ago

As usual, this is me writing out the architecture "on paper" before working on implementing it, as a way to reflect. Basically, rubber duck debugging. Feel free to ignore.

The current situation

What's currently happening is that the SET_X_PROPS that made sense for one dataset are being carried over to the other, where might not. This is a cause for bugs, because I can open a dataset, select something from the rows or columns (say, BackSPIN_level_7_group), go back to the datasetlist, then select a different datastat that does not have this row or column. The UI will still think it exists, but it doesn't, and now our code breaks in all kinds of unexpected ways. Now, I could add checks for this in all views, but I think that's the wrong solution to the problem.

A related problem is that storing the initial state in the redux stores creates all sorts of issues. First, makes assumptions for which columns and rows are present in the dataset, and they might not be (this was the cause of an earlier bug in Sparkline, where a row was assumed to be present in all datasets, but one did not posses it). Second, it means that for every change made to the component API, the reducers have to be updated too. What's preferable is that the actions/reducers just deal with the storage of state, and know as little about what's in it as possible. That way less code needs to be synchronised, and it will be easier to extend the store with more views.

In short: currently view state is leaking between datasets, we are making too many assumptions about which data is present, and we are putting responsibility for default settings in the reducers while it should be handled by the view components themselves.

Reducing assumptions about the data and shifting responsibilty for default settings

To fix the assumption-issue, I propose the following:

  1. Start with initialSparklineState, etc. being undefined,
  2. When a view is mounted or for some other reason passed an undefined view state, it dispatches its own defaults (it's not really possible to do this earlier, since we often needs to know what columns/rows are in a displayed dataset before choosing defaults).
    • if we create an "initialState() function for this and call that inside componentWillMount it will even happen before the initial render without any rerendering!
  3. When a view is unmounted, clear its state again. This is important to avoid getting wrong settings in the next view!

    Proper encapsulation

The above solution does not yet address one of the main issues we have, which is that these states are disgusting globals, leading to leaking state. What we can do to fix this is encapsulate the various view states into the individual datasets that they belong to (by which I mean the datasets as they are stored in the client side in the redux store, not the loom files). So, for example:

Every dataset keeps track of its own view state. Step 3 from before ("wiping" the view state when unmounting) can now be skipped, because the view states are encapsulated by the relevant datasets and no longer leak. This has the side benefit of storing our last view settings when we come back later!

Note that this requires that we update our actions/reducers and views as follows:

This is also directly related to #41. Below I'll write out how I want to handle that.

Encoding view settings in the URL

Recall that we use react-router to pass URL parameters to our components. So to give an example: /dataset/genes/cortex-zeisel-science-2015/cortex_5000.loom/<view settings> matches /dataset/genes/:project/:dataset(/:viewsettings) in such a way that our GenescapeView is passed a this.props.params, which contains:

{
  project: 'cortex-zeisel-science-2015',
  dataset: 'cortex_5000.loom',
  viewsettings: <view settings>,
}

The <view settings> are a placeholder for the url string encoding a genescapeState-object.

A problem this introduces is that we can have both a genescapeState from the url, and from the redux store. But the solution to this is quite simple: when navigating, the URL has priority, in all other situations, it doesn't. In practice that means that we listen to the <view settings> when mounting, and override any genescapeState already present in the store, and After mounting, the url should be synchronised to whatever changes to genescapeState we dispatch to the redux store.

So, for every view:

Currently our view state is stored in a bunch of leaky globals, which is a source of bugs. We should refactor that so that every dataset stores its own view state, and so that view components are responsible for their own defaults. Once this is implemented, we can build URL-encoded view settings on top of this with relative ease.

JobLeonard commented 8 years ago

Everything described above except view settings (which is a separate issue anyway) has been implemented with 34082b0717f8062cfac8962dc02f23d21e057cac