createTitles updateTitles parameter discrepancy

[r4j4h]

Hey guys,

I love dc.js, and while extending it I ran into a type issue. This may be something I am overlooking, but I've tracked it down and I think it's real. It's about a .title callback during a pie chart's lifecycle.

Basically, if you set a .title function, and watch the arguments passed to it, they differ.

From createTitles, it gets an object that includes data (containing the real key/value), startAngle, endAngle, and value.

From updateTitles, it only gets the data object.

This means that a function written to use d.key will break, getting undefined initially. The updateTitles function runs shortly after usually and covers this up, but it does seem to be happening.

dc.js/dc.js

Line 3816 in ebcd708

return _chart.title()(d);

It seems that changing
return _chart.title()(d);
to
return _chart.title()(d.data); fixes the issue.

[gordonwoodhull]

Thanks for the report! I agree, the accessor functions should always expect data, and can't really expect to do anything with the angles and so forth.

I'm making a slight correction to your post, as it looks like you pasted the same before/after code above. Please verify.

[r4j4h]

I can't see the edit that you made but it looks good. The change is just that d to d.data. I will fork and send a pull request in just a moment. :)

[gordonwoodhull]

See also #703 - more inconsistencies with .title

We should make this consistent throughout. Yuck. :-(

[gordonwoodhull]

So .title() is called even more inconsistently than .label():

gordon$ grep title\(\) src/*
src/bubble-mixin.js:        return _chart.title()(d);
...
src/geo-choropleth-chart.js:                return _chart.title()({key: key, value: value});
src/heatmap.js:                .text(_chart.title());
src/pie-chart.js:                return _chart.title()(d);
src/pie-chart.js:                    return _chart.title()(d.data);
src/row-chart.js:            rows.append('title').text(_chart.title());
src/row-chart.js:                        return _chart.title()(d);

I.e. choropleth agrees with piechart here, but I think everyone else is passing the "outer datum".

For future reference, line chart and bar chart both have

            *.append('title').text(dc.pluck('data', _chart.title(d.name)));

Which I think means (if I can parse this second- or third- order function) that they fall in the "inner datum" camp.

[r4j4h]

Wow, great finds! We definitely need to make this more consistent.

I think at the end of the day the critical difference is whether or not title/label functions should have access to the "outer datum data". That data usually is chart-specific and rendering state-specific --- e.g. angles for pies, layer names for stack charts, etc.

I think we got here because some charts obviously have little use, say maybe bar charts, so they just got passed the datum, while pie charts saw a potential use for having the angles around.

Perhaps we are missing an *Accessor function to handle the discrepancies when layers are around? Perhaps that could be added and all charts just assume 1 default layer for a consistent outer datum? Or perhaps it would be better to only have the title/label functions use the actually consistent inner datum (that will be driven by their dimension/grouping reduction so they should know what to expect from).

What are you feelings? I think we both agree it should be consistent. In #703 you mention you feel it should be done in 2.0 even at the cost of breaking compatibility. I am in agreement with that. But, I do not know which of the two directions we should move towards.

Should we continue more analysis, or start honing in on an action plan?

[gordonwoodhull]

My current thought is that since it's always more powerful to pass the outer datum, we should do that.

If my analysis is correct, this may be problematic because the most common charts, bar and line, are passing the inner datum. So it may break a lot of code. But it's the kind of interface change most programmers can fix in their sleep.

[r4j4h]

Yep, I agree completely, as long as there is a consistent way to access the inner datum regardless of the outer datum's structure.

[r4j4h]

I updated the branch that I had my title function change to pass the inner datum on. Our discussion here though lends to keeping it passing the outer datum itself (so not including that recent branch change), and changing other things to pass their outer datums instead.

I will ponder and perhaps start on that.

[gordonwoodhull]

I've decided not to make breaking changes for 2.0... Even though it's such a nice round number and it would be nice to have a clean interface, I think having it be stable is more important than having it make sense. 😉

So I'll pull the simple fix for 2.0 that makes it not broken, and consider making the data access consistent for 2.1.

[gordonwoodhull]

That's a good question whether there always is an outer datum. I think there always is something which belongs to the chart layout which then contains the datum from crossfilter, but it's worth double checking.

[gordonwoodhull]

I found a little bit of the history of the d / d.data controversy in this comment: #338 (comment)

The fix for that issue attempted to change all d to d.data but I guess it only got through a few charts.

On the one hand, d.data is more what the user would expect, given that they are supplying a crossfilter group or equivalent with key/values pairs. On the other hand, d is more powerful because it often contains other, computed data which may be helpful.

It is going to cause a lot of pain when we fix it either way, but I want to do this in 2.1.

A third possibility is to consistently always copy the crossfilterish data into an internal data structure, that contains key and value and whatever else is calculated. It still breaks anyone who is relying on d because they are surely referencing d.data, which would go away.

I'm pretty sure this is what I'd do if I were writing the library from scratch. Holding a reference to the crossfilter data is dangerously lazy; #1032 (comment) describes an anti-pattern in dc.js where code implicitly expects crossfilter to update its data in-place. Yuck!

[ghost]

I noticed this too...

label uses data and title does not...

.label(function(d) { return Math.round(d.data.value *100)/100 })
.title(function(d) { return Math.round(d.value *100)/100 })

Reference: https://groups.google.com/forum/#!topic/dc-js-user-group/qVGbZWWolCs