Coordinate Grid brushing staggered

[mtraynham]

Hey Gordon,

You added a recent change to the renderBrush function, but I think this actually makes the brushing event worse.

My proposal is to remove the call to redrawBrush from the _chart._brushing method at https://github.com/dc-js/dc.js/blob/develop/src/coordinate-grid-mixin.js#L913. It seems unnecessary to "redraw" the brush if the event itself is redrawing the brush. The change you made just amplifies this issue because the redraw now happens with a transition and likely during a user brush. Feel free to clarify if there is a reason it should be calling that method. It likely still needs to call the _chart.fadeDeselectedArea(); at https://github.com/dc-js/dc.js/blob/develop/src/coordinate-grid-mixin.js#L944.

[gordonwoodhull]

Hi @mtraynham. You are right, it shouldn't be transitioned when in response to a mouse event. I hadn't noticed that.

I'm not sure why the redrawBrush is there; it was introduced in 309a2f4 and apparently has something to do with event throttling.

I'll fix it one way or the other today.

[gordonwoodhull]

So... disabling that redrawBrush is not sufficient, as bits and pieces of the brush are still transitioned.

I've pushed a branch no-transition-brush which only transitions the brush in the chart resize/rescale case. Could you check that this works for you?

[gordonwoodhull]

I think the more general way to code this would probably be to pass down a d3 selection from the events that actually know whether transitions are needed or not - this selection would be either transitioned or not and all subselections would inherit from it. Rather than each piece of code starting from scratch with its own selection and deciding whether to transition or not.

It's kind of a big refactor, though.

[mtraynham]

Ehh sort of, I still see it happening with Composite charts, usually through this stack trace:

_chart._brushing()
_chart.redrawGroup()
drawChart()
_chart._prepareYAxis
prepareLeftYAxis()
chart.y()
_chart.rescale()
drawChart()

Alternatively, those could be fixed by changing the code to not reset the y scale and cause _resizing = true;:

    function prepareLeftYAxis() {
        if (_chart.y() === undefined) {
            _chart.y(d3.scale.linear());
        }
        if (_chart.elasticY()) {
            _chart.y().domain([yAxisMin(), yAxisMax()]).rangeRound([_chart.yAxisHeight(), 0]);
        }

        _chart.y().range([_chart.yAxisHeight(), 0]);
        _chart.yAxis(_chart.yAxis().scale(_chart.y()));

        _chart.yAxis().orient('left');
    }

    function prepareRightYAxis() {
        if (_chart.rightY() === undefined) {
            _chart.rightY(d3.scale.linear());
        }
        if (_chart.elasticY()) {
            _chart.rightY().domain([rightYAxisMin(), rightYAxisMax()]).rangeRound([_chart.yAxisHeight(), 0]);
        }

        _chart.rightY().range([_chart.yAxisHeight(), 0]);
        _chart.rightYAxis(_chart.rightYAxis().scale(_chart.rightY()));

        _chart.rightYAxis().orient('right');
    }

It did resolve for non-composite charts although this looks band-aid-ish.

[gordonwoodhull]

I see, the problem here being that setting the Y scale falsely registers as a rescale. This looks like a good fix because it will help toward supporting non-linear scales for elasticY. Thanks!

[mtraynham]

Not a problem. After trying my change out, everything on your no-transition-brush branch seems like it works fluently. Although, I'm still a bit lost on why we need to call that redrawBrush method from _brushing. _brushing usually triggers a redraw of the chart group, and the chart that was "brushed" will get redrawn thus invoking redrawBrush a second time.

[gordonwoodhull]

Aha, I think it is for brush rounding - the first line in _brushing calls extendBrush, which rounds the extent (e.g. to months). Then the brush needs to be redrawn.

So it's unrelated to the commit message, just another fix that went into the same commit.

[gordonwoodhull]

Wow, breaks 20 tests. Hold on.

[mtraynham]

Oof, which part the yAxis change? Or the optional redrawBrush?

[gordonwoodhull]

Aha:

        if (_chart.y() === undefined || _chart.elasticY()) {
            if (_chart.y() === undefined) {
                _chart.y(d3.scale.linear());
            }
            _chart.y().domain([yAxisMin(), yAxisMax()]).rangeRound([_chart.yAxisHeight(), 0]);
        }

etc.

[mtraynham]

Ahh good catch! Didn't set the domain for null undefined y scales.

[mtraynham]

Hey thanks Gordon! By the way, looking at the commit:
9b4242a#diff-220a8baefc3911d2f47ce06b66f53338R5988

Shows the old way I recommended. Not a big deal, just make sure you push the latest web build up when you commit next time 😄

[gordonwoodhull]

Hmm, I think this may be the fix for #808 biting us again.

Not building the docs by default can lead to lots of stuff out of sync.

[gordonwoodhull]

I also rebroke the resizing of composite charts, fixing this. 😦

Most likely transitions canceling each other out.

#974