BiGCZ: Only search if query has changed

[arottersman]

Overview

We were searching any time you made a catalog tab active. Now we only run a search if we've updated the query (or the search has never been run).

While looking through data catalog views, noticed we were setting className dynamically, which I've had trouble with in the past. Correcting this fixed the bug on IE11 where the tab contents would remain the same for all tabs (ie all tabs showed CINERGI results).

Connects #1960
Connects #2014

Demo

Testing Instructions

• Pull and bundle
• localhost:800?bigcz
  On the major browsers:
• Draw an AoI and search for something
• Confirm the search doesn't execute again when you switch between tabs, but does re-execute if you update the query (either date filters or query text)
• Confirm the pagination still works

[rajadain]

immediately repeated promise

I think you mean "immediately resolved promise"

[rajadain]

The IE11 code looks good. I can imagine some fail cases for searchIfNeeded. Going to test it out now.

[rajadain]

Consider lifting utils.formatBounds(bounds) out of this and search below so it is called less frequently.

[rajadain]

Consider the case when a search is already running. If a user were to change the query and re-search, !isSameSearch evaluates to true and we return this.searchPromise. However, when the first search returns, the always block executes and deletes self.searchPromise. What happens if this were to be deleted before the client can use its value?

[rajadain]

Ideally we would cancel this.searchPromise before starting a new one. Not sure how that can be done. Another alternative may be to chain the new search onto the old search.

If I'm misreading the code (which I could: promises are hard to follow), please do correct me.

[arottersman]

I will work out a way to explicitly cancel the ongoing promise for clarity.

I don't think, however, that delete self.searchPromise prevents the new promise from resolving correctly (and we don't use this.searchPromise anywhere else).

This is not what I'd expect the behavior to be, but here's a little test I made:

var self = this;
var d1 = $.Deferred();

this.myPromise = $.when(d1)
       .done(function (v) {
           console.log(v);
       })
       .always(function() {
	   console.log("deleting in first");
           delete self.myPromise;
       });

var d2 = $.Deferred();

this.myPromise = $.when(d2)
        .done(function (v) {
            console.log(v)
        })
        .always(function() {
	   console.log("deleting in second");
           delete self.myPromise;
        });

d1.resolve( "first done" );
d2.resolve( "second done" );

/** outputs
VM10771:6 first done
VM10771:9 deleting in first
VM10771:17 second done
VM10771:20 deleting in second
**/

[rajadain]

Found a bug. To reproduce:

• Search for "water" in CINERGI tab. Wait for results to complete
• Click WDC tab. Wait for results to complete
• Go back to CINERGI tab

Expected Result:

• WDC geometries are removed / hidden from the map, and CINERGI's are shown

Actual Result:

• WDC geometries are not removed, and CINERGI's are not shown

Demo:

[rajadain]

Should this connect #2014 instead of #2054? Seems to be making #2054 worse

[arottersman]

Should this connect #2014 instead of #2054? Seems to be making #2054 worse

Yes, that must have been a typo. Thanks! (Although maybe #2054 will be fixed when this done?)

[arottersman]

This should be ready for another look! That double fixup corrects a mistake I made while rebasing.

[rajadain]

Taking another look

[rajadain]

+1 tested. We're not excessively searching, and the tabs work correct on IE11. Also, great solution to accounting duplicate searches.

The only failing case I've been able to find is when there is an error (either due to a timeout or something else), I think we should allow the user to re-search, in case the error was due to an intermittent issue:

[arottersman]

Nice find — I've added a commit.

[arottersman]

Actually, this isn't going to work. this.searchPromise isn't a deferred object, and doesn't have a .reject() method. It's a jqXHR because that's what backbone returns with .fetch.

I think I'll change .reject to abort. Another option is to wrap the fetch in another promise deferred promise, but I think that makes it a little more difficult to follow.

[arottersman]

Added one more fixup to switch from .reject() to .abort() when cancelling the promise. Should be ready for another look now.

[rajadain]

Great work following up on this. Tested, works in all conditions.

[arottersman]

Thanks for reviewing! Squashed fixups (and fixed typo in that commit message). Will merge once the build completes.