Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly wait for Vega plots to be created before enabling various interactive things #85

Closed
fedarko opened this issue Apr 23, 2019 · 2 comments · Fixed by #166
Closed
Assignees
Labels
bug Something isn't working important Things that are critical for getting Qurro in a working/useful state optimization Making code faster or cleaner

Comments

@fedarko
Copy link
Collaborator

fedarko commented Apr 23, 2019

Currently, there is technically a small window of time where the plots are still being created by Vega* but none of my code is running (so the rrv UI is free to be used). It'd be a good idea to modify, e.g., makePlots() to make it explicitly asynchronous, and wait for the plots to be created.

In the meantime—before the plots would be created—the various UI elements in the app should be "disabled" (like in MetagenomeScope, sorta). I'd imagine that this would include the buttons for running text queries and taking screenshots, as well as the click events on the rank plot (... so just delay running addClickEventToRankPlotView()). Once the plots have finished loading, we could enable the UI elements.

@fedarko fedarko added the enhancement New feature or request label Apr 23, 2019
@fedarko fedarko self-assigned this Apr 23, 2019
@fedarko fedarko changed the title Explicitly await for Vega plots to be created before enabling various interactive things Explicitly wait for Vega plots to be created before enabling various interactive things Apr 23, 2019
@fedarko
Copy link
Collaborator Author

fedarko commented Apr 23, 2019

As eventual proof that this works: we should be able to programmatically create an RRVDisplay object and then call its destroy() method in the next line without any problems. (I actually tried to test the destroy() method and that failed for this reason -- which is why I stumbled upon this issue.)

See: (to be added eventually to test_rrvdisplay.js)

    describe("The RRVDisplay destructor (destroy())", function() {
        var rrv = new display.RRVDisplay(rankPlotJSON, samplePlotJSON);
        rrv.destroy();
        it("Properly clears DOM element bindings", function() {
            for (var i = 0; i < rrv.elementsWithOnClickBindings.length; i++) {
                chai.assert.isUndefined(
                    document.getElementById(rrv.elementsWithOnClickBindings[i]).onclick
                );
            }
        });
        it("Properly clears the #rankPlot and #samplePlot divs", function() {
            chai.assert.isEmpty(document.getElementById("rankPlot").innerHTML);
            chai.assert.isEmpty(document.getElementById("samplePlot").innerHTML);
        });
    });

fedarko added a commit that referenced this issue Apr 23, 2019
This way we only have to modify the invocation of setUpDOMBindings()
to add more onclick events.

Also added a test to validate that this works properly. (I was going
to add a test to validate that destroy() works properly but then I
ran into #85.)
@fedarko
Copy link
Collaborator Author

fedarko commented Apr 25, 2019

Yeah, I think this is a prerequisite before we can write JS tests that involve checks on the actual view objects.

Here's another test that should eventually be added to the RRVDisplay tests (under the "Multi-feature selections" describe block), but currently triggers a TypeError because it calls this.samplePlotView.change() when this.samplePlotView is still undefined:

                describe("Text searching", function() {
                    document.getElementById("topSearch").value = "text";
                    document.getElementById("botSearch").value = "text";
                    document.getElementById("topText").value = "Taxon";
                    document.getElementById("topText").value = "Yeet";
                    rrv.updateSamplePlotMulti();
                    it("Properly updates topFeatures and botFeatures", function() {
                        chai.assert.sameMembers(
                            [
                                "Taxon1",
                                "Taxon2",
                                "Taxon3|Yeet|100",
                                "Taxon4",
                                "Taxon5"
                            ],
                            rrv.topFeatures
                        );
                        chai.assert.equal(["Taxon3|Yeet|100"], rrv.botFeatures);
                    });
                });

@fedarko fedarko added bug Something isn't working important Things that are critical for getting Qurro in a working/useful state optimization Making code faster or cleaner and removed enhancement New feature or request labels Apr 25, 2019
fedarko added a commit that referenced this issue Apr 26, 2019
This will make it easier to unit-test this function, due to #85
restrictions
fedarko added a commit that referenced this issue Apr 26, 2019
We're blocked in from actually doing this by #85 -- since
getSamplePlotData() involves querying the samplePlotView, we need
to be able to rely on that view uh existing when we run this test.

So #85 is actually a pretty high priority thing, then.
fedarko added a commit that referenced this issue Jun 6, 2019
I didn't realize that RRVDisplay.changeSamplePlot() updated the
sample plot JSON (which is passed in as the spec when vegaEmbed()
is called), but that it does 1) makes sense and 2) is useful.

This will let us unit-test getSamplePlotData() before #85, which
will be sufficient to address most of my concerns with #87.
fedarko added a commit that referenced this issue Jun 6, 2019
Ideally we'd also test data exporting *after* user-initiated
feature selection, but since #85 is blocking that this is the next
best thing. This gives us confidence that #76 works as intended.

(Not done with #76 just yet; I'd still like to add functionality
for exporting rank plot information re: selected features.)
fedarko added a commit that referenced this issue Jun 6, 2019
progress on #92 (obvs need more thorough testing of corner cases)
and on #85 (or at least on making clear the amount of work left to
be done, which is nontrivial).
fedarko added a commit that referenced this issue Jun 12, 2019
This sums up the programming work for #92, mostly. What I want to
do now is add tests, etc. to make sure
RRVDisplay.updateSamplePlotFilters() is working properly before I
merge this back in. Also thinking about maybe working on #66 while I'm
at it.

Probably a good time to take care of #85 et al. also?
fedarko added a commit that referenced this issue Jun 13, 2019
Need to add tests that this works as expected (...and that this
actually lets us test things properly), as well as maybe disabling
UI elements during these computations. But this is exciting!

Also added a mini loading message which kind of fulfills #108 but
not really.
fedarko added a commit that referenced this issue Jun 15, 2019
This closes #85. The issue of making sure the user can't click stuff
before things are loaded is addressed in #108 and maybe #129.

Interesting stuff:
-Turns out setting document.getElementById(e).onclick = undefined;
 just STRAIGHT UP DOESN'T WORK, and results in .onclick being set
 to null. I kept thinking my code was being run out of order or
 something -- and that this was an error with async/await stuff --
 but this was literally a problem with setting it to undefined.
 So setting it to null solves this.

-Apparently doing a ton of intensive stuff in a constructor is
 frowned upon. So I changed things up a bit so that makePlots()
 actually "initializes" most of the important stuff in the
 RRVDisplay (including setting up the DOM, which is now done in
 setUpDOM(), easily enough).

-Switched to using await Promise.all() instead of multiple awaits;
 this way is cloesr to what we want IMO

-Added destroy() tests! This was the main proof-of-concept I wanted
 that #85 was taken care of :)
fedarko added a commit that referenced this issue Jun 16, 2019
This is the sort of thing we needed to knock out #160 (... and #85)
for.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working important Things that are critical for getting Qurro in a working/useful state optimization Making code faster or cleaner
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant