Initial version of zoom with _zoomOutRestrict using D3 zoom features

[kum-deepak]

This is work in progress. Please check using http://localhost:8888/web/zoom/restrict-panning.html

I need suggestions at this stage:

• I am assuming that in a range/focus chart pair will have same domains. Are you aware of any situation where it will not be the case?
• In the D3v4, setting a zoom programmatically calls the event handler as well, to avoid infinite loop, there is check in the beginning of the event handler (picked from D3 examples). Same case is there for brushing as well.
• Now we get a peculiar situation when a range chart tell the focus chart to update which it turn tells the range chart. In previous version it was checking if the previous filter/focus values were same according intimate the other chart. In newer D3 API, the zoom and brush events return screen measurements (i.e. scale.range()) which needs to be converted to domain values. This very rarely (combined with the fact that computations are in floating point) gives slightly different values than the original - leading to infinite loop. I was thinking to introduce an optional 2nd parameter to few calls (for example: _chart.focus) to indicate that do not issue further intimations to other charts. Do you have any other thoughts?

The above gives slightly peculiar UI glitches because some updates get applied immediately while some are debounced.

Quite a lot of code has changed, I will refactor before making final commit :)

[kum-deepak]

I have completed work that I had targeted for this iteration. Will wait for your feedback.

I believe that if we carefully check what we debounce and switch off transitions during zoom/brush, we can do updates online. Once this PR is accepted, I will do a PoC - based on the results we may decide to merge that one. If that works it will give a much better user experience.

For now I will move onto some other task.

[gordonwoodhull]

Wow, this is a valiant effort. I can see that it already works better than the old version, where it was possible to keep panning off the edge. (It wouldn't display out of bounds, but you'd have to pan all the way back for panning to start working again.)

It's a pity about all those "don't fire event" flags. I am familiar with the infinite recursion of events as it happens in a lot of UI. Sometimes there is a good way to refactor, sometimes no. I am going to look closely at this before merging.

[kum-deepak]

Currently I have introduced a second argument to the .focus call in a backward compatible way. This should suffice to avoid infinite recursion. I will really like to avoid it, however, could not figure out a reliable way.

[gordonwoodhull]

If this is the same as rescaleX, why not just use rescaleX?

[gordonwoodhull]

I'm diving into this today. Feels like we're fighting with d3.zoom a lot here, and I really hope there is a simpler way. I'm posting questions as they come up, hope to clean this up and merge it today. (no need for you to update the PR)

[gordonwoodhull]

This seems to work afaict:

        var newDomain = event.transform.rescaleX(_origX).domain();

[kum-deepak]

I think it would. :) The more I am getting to know D3 the more I admire.

I reached this code independently step at a time (using drawings on a paper with pencil). Just now rechecked - this code seems to do exactly the same. Let me know if you would change or should I.

[gordonwoodhull]

Okay, great. I am working on it.

I hope to also clean up the recursive event stuff and merge this today. (The zoomHandler argument makes sense to me, but setting d3.event seems messy so I'm looking at using the same strategy there.)

[kum-deepak]

The d3.event, I could not understand their reasoning behind keeping the event in a global variable. I think it would have been much cleaner design for D3 authors as well as users if D3 passed the event as parameter to event handler (or, it already does and I am unaware).

I had started setting it to null as I was getting occasional infinite loop. Maybe those were caused by something else. D3 examples don't do that.

[gordonwoodhull]

I don't understand the global either, and I don't remember seeing a rationale for it. It is what it is. 😉

Yes, it's easy to repro the infinite recursion. I'm experimenting with using arguments similar to noRaiseEvents to fix this. Still makes me think that we might have things inside-out, at least for the new way that d3.zoom works.

However, it's likely that the problem is abstraction itself: often things that are easy in ad-hoc UI code become really complicated when you try to make the code reusable. A lot of people reject dc.js for this reason, but I still think it's worth it!

[kum-deepak]

Actually I am not sure about others, if it was not for dc, I would not dare to build the functionality that I need using D3 directly. D3 is like an ocean for a new person.

So definitely worth it.

[gordonwoodhull]

Made good progress last night on the recursion and general cleanup - optimistic that I'll finally merge today.

That's really cool when you work out something with pencil and paper, and it turns out you've rediscovered something in the library. I bet that domainToZoomTransform is in d3.zoom somewhere too, but I haven't found it yet.

It's great how much special-purpose code you were able to eliminate here, because d3.zoom is that much more powerful (or we weren't using it right). There is a similar situation in dc.barChart which recreates a lot of things in d3v3 rangeBands and d3v4 scaleBand - it will be a mess to fix it in a backward-compatible way, but a huge relief once all that custom code is gone.

[kum-deepak]

The zoom and brushing were the biggest changes I undertook. Great to see that it is reaching closure. 👍

I understand the code that you mean in dc.barChart I will give it a shot :) I did realize that it has phrases copied from D3 code.

[gordonwoodhull]

Yes, this is a huge change and well done. Just the event recursion needs to be fixed. I'm closer but still haven't quite figured out the right strategy.

As for bar charts, I don't think anything was copied from d3; I think it's an independent implementation. I misspoke, ordinal-scale charts do use rangeBands, but quantitative-scale charts use something homemade, see _gap etc.

Potential problem: bars can be of varying width, see e.g. a bar chart aggregated by month. I don't know if d3v4 has a new way to do that. I've never thought it was a great idea, but people apparently rely on it. It's natural to want to use a d3 time scale for the X axis, so that precludes rangeBands or scaleBand.

[gordonwoodhull]

Hi @kum-deepak, I fixed the recursion problem by using a reference count - whenever we're in a section which we think should not generate events, we increment a counter, and the event is not processed if the counter is greater than zero. This is commit 3f16842 on the merge1385 branch.

It's not all that elegant but it says exactly what it means and it doesn't modify d3. I continue to think we may be able to structure our code better, so that by design we do not set the brush in response to the brush, or the zoom in response to the zoom, but this works.

I know you are occupied with other things, but in case you have a couple minutes, could you take a look at the failing test cases on this branch? (build log)

The first two may not matter, since we are just applying zoom extent later:

• dc.coordinateGridChart restricting zoom out should set the start of zoom scale extent to 1
• dc.coordinateGridChart disabling zoom out restriction should set the start of zoom scale extent to 0

But these two seem to suggest that zoomOutRestrict does not work in all cases

• dc.coordinateGridChart with zoom restriction enabled should not be able to zoom out past its original x domain
• dc.coordinateGridChart with zoom restriction enabled with a range chart should not be able to zoom out past its range chart origin x domain

If you have any thoughts, please let me know. Otherwise I will try to fix them tomorrow (and finally merge this!)

[kum-deepak]

I will have some time later in the day today (your time tomorrow morning) I will have a look. 👍

[kum-deepak]

Hi @gordonwoodhull, went through your commit. I find that you are trying to restrict more than it should. I am not quite happy with the solution. If it is ok with you, I will make one more attempt. In my previous analysis I had found that I needed to distinguish a (d3) event generated - in response to a user action or a programmatic call. When a d3.event is generated and event handler is called in response to a programmatic calls - we do not need to execute the event handlers (the return from the top of the function).

In this attempt I will go through the D3 event documentation in detail - may be skim their code. Go through their sample code again. In my last attempt I was trying to solve many moving things, in this one it is only one. I am quite hopeful to find a solution without making an unacceptable call onto d3.event.

Will you be willing to wait for the weekend on this one?

[gordonwoodhull]

Sure, I'm open to other solutions - it was mostly the setting of d3.event which I thought was dangerous (and probably causes #1396).

Keep in mind that your original PR has the same exact errors - so I don't think I changed the behavior. I think it's the same idea - the handler sets something so that if recursion hits the same spot deeper in the call stack, it bails out. My technique was simply, we are the caller of d3 so we know we want to ignore any events generated deeper in the same call stack.

No rush.

[kum-deepak]

Now I have understood how d3 is setting up event. When setting up an event if there is already a d3.event, it is assigned to event.sourceEvent of the newly created event. No consideration is made if it was actually source for the event. After the event handler finishes it assigns the d3.event to the previous value.

So, from within an event handler, if any call would directly or indirectly generate an event, it will set the current event as sourceEvent.

I have gone through their examples, each example uses their specific conditions (e.g., https://bl.ocks.org/mbostock/6232537 and https://bl.ocks.org/mbostock/6232620) to detect programmatic event vs user initiated.

Generally for brushing, the sourceEvent will be one of the mouse or touch events when our handler first gets called. The recursive call will have of the d3 generated events as source event. This is my basis for check to terminate recursion.

With that understanding updated specs - simulated brush events - now clears d3.event when the handler finishes.

I have committed changes for brushing, next target for zoom. :)

[gordonwoodhull]

Thanks @kum-deepak!

This is great - it solves the recursion problem without resorting to setting d3.event, and it's very clear how it distinguishes between programmatic and "organic" (user-response) events. I'm also very happy to learn how to make that distinction in general! Level up.

One small ask: would you please re-apply the function declaration changes I made? To me

var _f = function(...) { ... };

means _f is a private property which will change, usually in response to a setter. Whereas

function f(...) { ... }

means f is a private method which will not change at runtime. Using

grep 'var.*= function' src/*

I see that our codebase is only about 90% consistent with my understanding, but I don't know how to write a jshint/jscs rule for this. I know that many JS developers prefer to use use var f = instead of function f() {} to declare a function but I think there is a helpful distinction to be made here.

Also, the same tests are still failing, but maybe the zoom fixes will help with that.

[gordonwoodhull]

Also, you dropped the commit messages on the fixes of mine you took in "Minor cleanup"
328e1d8. I find commit messages really helpful when looking at the history, so I'll probably restore them when merging.

[kum-deepak]

Reapplied the function declaration changes.

There are about 10 such cases in base and core. Outside of these around 20 cases. Create a task if I should make an attempt to convert all these.

[kum-deepak]

Sorry about the commit message, my code had gotten thoroughly messed up, so started from 3.0 and started applying code in different order than original commits (by copying and pasting). So, actually final commit-set does not have all the commits, while code is nearly the same.

If you find anything else missing, just let me know.

[gordonwoodhull]

OK great, glad we're on the same page. I'm still on my walk but I'm looking forward to continuing my review when I get back.

Looks like this is almost ready to merge, too, except for those zoom out tests. Good work!

[kum-deepak]

Pushed last commit for today :)

[kum-deepak]

How is your health, hope much better now 👍

[kum-deepak]

Done with current set of changes :) Let me know if any blockers are there for merge.

[kum-deepak]

Learnt another hard lesson (again) about global variables :) If a test case involving simulated brushing failed, some other test may mysteriously fail. Later figured out it was happening because d3.event was not cleared and D3 might use that some other purpose somewhere :)

I think we should raise a ticket in D3 requesting them to (gradually) remove d3.event and pass event as callback parameter.

[gordonwoodhull]

Looks like the rationale is consistent callback signatures? d3/d3#1887

[gordonwoodhull]

Merged! Thanks @kum-deepak!

[kum-deepak]

I realized that 2 minor commits (the recent ones) did not get merged. I will create a small PR with those. :)

[gordonwoodhull]

I just saw that - already fixed. I had merged earlier version of branch by accident!

[gordonwoodhull]

And thanks for asking after my health. I am doing very well but I still have one arm in a sling. Looking forward to being able to touch-type in a couple weeks!