dasmoth / dalliance

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

Gracefully handle setLength before currentSeqMax is set #70

Closed mdrasmus closed 10 years ago

mdrasmus commented 10 years ago

A infinite loop appears to occur sometimes if the following is done. I often have to force close my browser as a result.

var dalliance = new Browser({
    chr: "chr17",
    viewStart: 41180051,
    viewEnd: 41309898,

    coordSystem: {
        speciesName: 'Human',
        taxon: 9606,
        auth: 'GRCh',
        version: '37',
        ucscName: 'hg19',
    },
    sources: [
    {
        name: 'Genome',
        twoBitURI: 'https://www.biodalliance.org/datasets/hg19.2bit',
        tier_type: 'sequence',
        provides_entrypoints: true,
        pinned: true
    },
    ]
});

dalliance.setLocation("chr17", 31180051, 31309898);

Looking here it seems that we have newChr=null, because the new chromosome is the same as the old one. Therefore, this.currentSeqMax will not be updated by this.getSeqInfo().length (I know getSeqInfo is async, but you get the idea). Since its not updated, it is -1 by default which seems to cause problems for the following code in _setLocation(), which I haven't fully investigated.

The fix seems to also check here whether this.currentSeqMax > 0, because if it still equals -1 then we need to fetch seqinfo as well.

I'm happy to make a quick PR to fix this.

dasmoth commented 10 years ago

Yes, looks like there's a problem if you call setLocation too soon. I'm not 100% sure why you'd want to do this (as opposed to just passing the coords you want to the Browser constructor), but would certainly be good to get this fixed -- pull request welcome if you've got something ready.

mdrasmus commented 10 years ago

Also I don't know if you want this code to somehow be completed first before notifying initListeners.

Adding an initListener was my first attempt to address this issue. I was hoping this would allow everything to load before starting to change location view.

I'm not 100% sure why you'd want to do this (as opposed to just passing the coords you want to the Browser constructor)

Yeah, I gave a really simplified example. I am using the browser as part of a larger piece of code and the initialization part was done separately from first setLocation call.

dasmoth commented 10 years ago

Re: timing of initListeners... that's a tricky one. We've been trying to keep Browser instances without a configured Sequence Source usable, for the benefit of those who want really stripped down instances (probably with just a single track) based on minimal data. But it possibly needs a bit more thought about exactly what the rules for using such an instance should be.

mdrasmus commented 10 years ago

We've been trying to keep Browser instances without a configured Sequence Source usable, for the benefit of those who want really stripped down instances (probably with just a single track) based on minimal data. But it possibly needs a bit more thought about exactly what the rules for using such an instance should be.

Ok, keeping minimal requirements sounds good. Then, the change I have in #71 it probably fine for now.

dasmoth commented 10 years ago

Thanks for pull request.