dasmoth / dalliance

Interactive web-based genome browser.
http://www.biodalliance.org/
BSD 2-Clause "Simplified" License
226 stars 68 forks source link

refactor tier data loading to support custom renders. #148 #154

Closed Traksewt closed 9 years ago

Traksewt commented 9 years ago

allows for the tier data to be retrieved without rendering the default tiers. The default tiers can be rendered when needed by calling cbrowser.refresh(), which will use the cached data. updating the status and drawing the tier is now part of the default tier render method. Updating the tier height incase of exception was not needed as it was happening at the end of draw()->paint().

dasmoth commented 9 years ago

Looks okay except that the call to _notifierToStatus has been removed -- this will break visible error reporting (which I know has a pretty odd data-flow at the moment, but does work). Would it cause you any problems if I reinstated this when I merge?

Or do you need the tiers to be completely "headless"?

Traksewt commented 9 years ago

Notifytostatus should get called by the default tier renderer when it calls updateStatus. That way you can do what you like with the status for custom renderers. On 22/04/2015 4:27 PM, "Thomas Down" notifications@github.com wrote:

Looks okay except that the call to _notifierToStatus has been removed -- this will break visible error reporting (which I know has a pretty odd data-flow at the moment, but does work). Would it cause you any problems if I reinstated this when I merge?

Or do you need the tiers to be completely "headless"?

— Reply to this email directly or view it on GitHub https://github.com/dasmoth/dalliance/pull/154#issuecomment-95045698.

dasmoth commented 9 years ago

Okay, understand now -- makes sense.

Medium-term, I wonder if it might make more sense to start allowing subclassing of the DasTier prototype (which probably shouldn't be called that any more...)

Traksewt commented 9 years ago

Thanks. I started a branch where I split Tier class into Tier (for the data) and TierRenderer for the GUI parts. This is to support multiple renderers from the one tier. I don't want to replace the linear dalliance render, just add a new visualisation option.

I had a list of TierRenderers in the cbrowser.

It got tricky splitting the tier config as some of the config was for the data, some for the style. Some style items I'd be happy to be consistent (i.e. available in Tier), like feature colour. So I put it on the backburner, and focused on the data retrieval refactor.

Even if we go down this path and allow for multiple TierRenderers to listen off one Tier (data update), we would still have the case where we don't want all TierRenderers to render at once (e.g. if one is offscreen in a tabbed pane), so we'd need flexibility on selecting which TierRenderers to trigger off a particular data retrieval.

On Thu, Apr 23, 2015 at 5:14 AM, Thomas Down notifications@github.com wrote:

Okay, understand now -- makes sense.

Medium-term, I wonder if it might make more sense to start allowing subclassing of the DasTier prototype (which probably shouldn't be called that any more...)

— Reply to this email directly or view it on GitHub https://github.com/dasmoth/dalliance/pull/154#issuecomment-95307942.

dasmoth commented 9 years ago

I don't think I like the idea of multiple TierRendererers per Tier. There's already a caching system in place which means that if two Tiers have the same source configuration (see sourceConfigsAreEqual etc.), they'll be backed by a single data fetcher (with appropriate routing of data), so in general I think having extra Tier objects is probably the right answer. We might need to allow tiers to be "headless" (e.g. don't have a viewport in the main browser component).

Having different sets of tiers fetching at different times becomes trickier... but here's a thought: what if we allowed multiple KnownSpace objects, potentially associated with different sets of tiers. Each distinct view has its own KnownSpace. That was actually the original plan when I introduced KnownSpaces -- I've also had an eye on supporting split-screen viewers (e.g. see two interacting genomic regions side by side), which would be two KnownSpaces with (usually) the same set of Tiers.

This does get kind-of complicated, though.

Traksewt commented 9 years ago

Does the single data fetcher allow for multiple tiers retrieving data from the same source but looking at different locations, to handle requesting for both locations at once? If so, that is great, it would be useful for my other scenarios.

In my case at the moment, I want the tiers with different rendering to be in sync, as I am showing the same data, just in a different way. So they can share the knownspace (which will happen anyway) and all data retrieved, it just needs to render it differently. Hence ideally I'll just register the callbacks once, as I care for when the data is loaded/changed, not when it is visualised.

Parts of your solution is common to mine, as making tier headless was what I was trying. If it is headless, then the viewport needs to go somewhere, which sounds like it would attach to your view idea? Would the default tier rendering be a subclass of the basic Tier, or does it remain as it is, and I overwrite the rendering methods for my custom rendering in a subclass? Either way, in this design each of the tiers would hold a reference to the features/sequence even if they are the same (though using the same knownspace they will both point to the same feature array). And they would have separate listener lists.

This can work, just a design decision on whether to separate model/view (and refactor the rendering to use the view) or subclass. I'm easy either way.

One thing I'm not clear on, is how does the KnownSpace know to use the same featureCache for different tiers with the same config? It looks like it is keyed by the tier object itself. I would have thought that featureCache would need to be keyed by a hash (or unique name) of the dasSource. kspace.js ln 241 var baton = thisB.featureCache[tier];

I can't see reference to sourcesAreEqual in kspace. Thanks.

dasmoth commented 9 years ago

Yes, multiple KnownSpaces sharing fetchers should be absolutely fine.

The featureCache in KnownSpace is separate from the layer which routes data from a single fetcher to multiple tiers (see CachingFeatureSource and the top of sourceadapters.js). In some senses, the cache in KnownSpace might be a little wasteful (and I have, on occasions, pondered whether there might be ways to get rid of it entirely), but since -- in the two-tiers-share-single-fetcher case -- they'll both be getting the same set of objects back, it's not actually wasting much memory.