Skip to content

Pie aggregation and event cleanup #2117

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Oct 26, 2017
Merged

Pie aggregation and event cleanup #2117

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
679ed9c
remove obsolete comments, esp. about 3d pies
alexcjohnson Oct 24, 2017
ce8946c
first cut at aggregated pies
alexcjohnson Oct 24, 2017
d2756f1
lint pie
alexcjohnson Oct 24, 2017
ebc4d8b
:hocho: my zeroed-out 3D pie code
alexcjohnson Oct 24, 2017
5d0f3e2
standardize pie events, and add internal aggregation info
alexcjohnson Oct 25, 2017
e08e37a
simplify sankey hover events
alexcjohnson Oct 25, 2017
4ba0249
rename and document appendArrayMultiPointValues
alexcjohnson Oct 25, 2017
ee1f8c4
jasmine test of pie agg with or without values
alexcjohnson Oct 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 46 additions & 10 deletions src/components/fx/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,22 +100,58 @@ exports.appendArrayPointValue = function(pointData, trace, pointNumber) {

for(var i = 0; i < arrayAttrs.length; i++) {
var astr = arrayAttrs[i];
var key;
var key = getPointKey(astr);

if(astr === 'ids') key = 'id';
else if(astr === 'locations') key = 'location';
else key = astr;
if(pointData[key] === undefined) {
var val = Lib.nestedProperty(trace, astr).get();
var pointVal = getPointData(val, pointNumber);

if(pointVal !== undefined) pointData[key] = pointVal;
}
}
};

exports.appendArrayPointValues = function(pointData, trace, pointNumbers) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe appendArrayMultiPointValue or appendArrayMultiPointValues would more typo-proof.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and please add jsDOC ⤴️

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call on appendArrayMultiPointValues -> with docs in
4ba0249

var arrayAttrs = trace._arrayAttrs;

if(!arrayAttrs) {
return;
}

for(var i = 0; i < arrayAttrs.length; i++) {
var astr = arrayAttrs[i];
var key = getPointKey(astr);

if(pointData[key] === undefined) {
var val = Lib.nestedProperty(trace, astr).get();
var keyVal = new Array(pointNumbers.length);

if(Array.isArray(pointNumber)) {
if(Array.isArray(val) && Array.isArray(val[pointNumber[0]])) {
pointData[key] = val[pointNumber[0]][pointNumber[1]];
}
} else {
pointData[key] = val[pointNumber];
for(var j = 0; j < pointNumbers.length; j++) {
keyVal[j] = getPointData(val, pointNumbers[j]);
}
pointData[key] = keyVal;
}
}
};

var pointKeyMap = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ at some point we might want to move this to the attribute declarations.

ids: 'id',
locations: 'location',
labels: 'label',
values: 'value',
'marker.colors': 'color'
};

function getPointKey(astr) {
return pointKeyMap[astr] || astr;
}

function getPointData(val, pointNumber) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This thing is very similar to Lib.castOption. I wonder if we could combine them? Note that getPointData should not fallback to trace-wide properties.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the multipoint case it's nice to keep the nestedProperty call out of this function, as that would slow it down dramatically. Between that and the fact that, as you mention, this one does not return trace-wide properties (it would return undefined but in fact it never gets there as it's iterating over arrayAttrs) these functions seem fairly distinct - though perhaps Lib.castOption could call this one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it might be nice to move getPointData to Lib at some point, but let's keep this as is for now.

if(Array.isArray(pointNumber)) {
if(Array.isArray(val) && Array.isArray(val[pointNumber[0]])) {
return val[pointNumber[0]][pointNumber[1]];
}
} else {
return val[pointNumber];
}
}
2 changes: 1 addition & 1 deletion src/components/fx/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ exports.loneHover = function loneHover(hoverItem, opts) {

// The actual implementation is here:
function _hover(gd, evt, subplot, noHoverEvent) {
if((subplot === 'pie' || subplot === 'sankey') && !noHoverEvent) {
if(subplot === 'sankey' && !noHoverEvent) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't see any purpose to having pie call Fx.hover other than throttling - which really shouldn't be an issue for pie, so I removed it from this special handling. Am I missing anything? Should we do the same for sankey and 🔪 this block entirely?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like sankey does Fx.hover here and here, but if you feel adventurous, sure, go ahead and patch sankey/plot.js to 🔪 this block.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplified sankey -> e08e37a
Also: I notice that the way we had it was only throttling event emission, not the actual item selection which is normally the slow part. Again, throttling shouldn't be an issue for either of these trace types, this is just more confirmation of that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e08e37a is very nice. 👌

gd.emit('plotly_hover', {
event: evt.originalEvent,
points: [evt]
Expand Down
41 changes: 41 additions & 0 deletions src/traces/pie/event_data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* Copyright 2012-2017, Plotly, Inc.
* All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/


'use strict';

var appendArrayPointValues = require('../../components/fx/helpers').appendArrayPointValues;


// Note: like other eventData routines, this creates the data for hover/unhover/click events
// but it has a different API and goes through a totally different pathway.
// So to ensure it doesn't get misused, it's not attached to the Pie module.
module.exports = function eventData(pt, trace) {
var out = {
curveNumber: trace.index,
pointNumbers: pt.pts,
data: trace._input,
fullData: trace,
label: pt.label,
color: pt.color,
value: pt.v,

// pt.v (and pt.i below) for backward compatibility
v: pt.v
};

// Only include pointNumber if it's unambiguous
if(pt.pts.length === 1) out.pointNumber = out.i = pt.pts[0];

// Add extra data arrays to the output
// notice that this is the multi-point version ('s' on the end!)
// so added data will be arrays matching the pointNumbers array.
appendArrayPointValues(out, trace, pt.pts);

return out;
};
127 changes: 73 additions & 54 deletions src/traces/pie/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ var Drawing = require('../../components/drawing');
var svgTextUtils = require('../../lib/svg_text_utils');

var helpers = require('./helpers');
var eventData = require('./event_data');

module.exports = function plot(gd, cdpie) {
var fullLayout = gd._fullLayout;
Expand Down Expand Up @@ -69,15 +70,23 @@ module.exports = function plot(gd, cdpie) {
var cy = cd0.cy;
var sliceTop = d3.select(this);
var slicePath = sliceTop.selectAll('path.surface').data([pt]);
var hasHoverData = false;

function handleMouseOver(evt) {
evt.originalEvent = d3.event;
// hover state vars
// have we drawn a hover label, so it should be cleared later
var hasHoverLabel = false;
// have we emitted a hover event, so later an unhover event should be emitted
// note that click events do not depend on this - you can still get them
// with hovermode: false or if you were earlier dragging, then clicked
// in the same slice that you moused up in
var hasHoverEvent = false;

function handleMouseOver() {
// in case fullLayout or fullData has changed without a replot
var fullLayout2 = gd._fullLayout;
var trace2 = gd._fullData[trace.index];

if(gd._dragging || fullLayout2.hovermode === false) return;

var hoverinfo = trace2.hoverinfo;
if(Array.isArray(hoverinfo)) {
// super hacky: we need to pull out the *first* hoverinfo from
Expand All @@ -95,68 +104,78 @@ module.exports = function plot(gd, cdpie) {

// in case we dragged over the pie from another subplot,
// or if hover is turned off
if(gd._dragging || fullLayout2.hovermode === false ||
hoverinfo === 'none' || hoverinfo === 'skip' || !hoverinfo) {
Fx.hover(gd, evt, 'pie');
return;
}

var rInscribed = getInscribedRadiusFraction(pt, cd0);
var hoverCenterX = cx + pt.pxmid[0] * (1 - rInscribed);
var hoverCenterY = cy + pt.pxmid[1] * (1 - rInscribed);
var separators = fullLayout.separators;
var thisText = [];
if(hoverinfo !== 'none' && hoverinfo !== 'skip' && hoverinfo) {
var rInscribed = getInscribedRadiusFraction(pt, cd0);
var hoverCenterX = cx + pt.pxmid[0] * (1 - rInscribed);
var hoverCenterY = cy + pt.pxmid[1] * (1 - rInscribed);
var separators = fullLayout.separators;
var thisText = [];

if(hoverinfo.indexOf('label') !== -1) thisText.push(pt.label);
if(hoverinfo.indexOf('text') !== -1) {
var texti = helpers.castOption(trace2.hovertext || trace2.text, pt.pts);
if(texti) thisText.push(texti);
}
if(hoverinfo.indexOf('value') !== -1) thisText.push(helpers.formatPieValue(pt.v, separators));
if(hoverinfo.indexOf('percent') !== -1) thisText.push(helpers.formatPiePercent(pt.v / cd0.vTotal, separators));

var hoverLabel = trace.hoverlabel;
var hoverFont = hoverLabel.font;

Fx.loneHover({
x0: hoverCenterX - rInscribed * cd0.r,
x1: hoverCenterX + rInscribed * cd0.r,
y: hoverCenterY,
text: thisText.join('<br>'),
name: hoverinfo.indexOf('name') !== -1 ? trace2.name : undefined,
idealAlign: pt.pxmid[0] < 0 ? 'left' : 'right',
color: helpers.castOption(hoverLabel.bgcolor, pt.pts) || pt.color,
borderColor: helpers.castOption(hoverLabel.bordercolor, pt.pts),
fontFamily: helpers.castOption(hoverFont.family, pt.pts),
fontSize: helpers.castOption(hoverFont.size, pt.pts),
fontColor: helpers.castOption(hoverFont.color, pt.pts)
}, {
container: fullLayout2._hoverlayer.node(),
outerContainer: fullLayout2._paper.node(),
gd: gd
});

if(hoverinfo.indexOf('label') !== -1) thisText.push(pt.label);
if(hoverinfo.indexOf('text') !== -1) {
var texti = helpers.castOption(trace2.hovertext || trace2.text, pt.pts);
if(texti) thisText.push(texti);
hasHoverLabel = true;
}
if(hoverinfo.indexOf('value') !== -1) thisText.push(helpers.formatPieValue(pt.v, separators));
if(hoverinfo.indexOf('percent') !== -1) thisText.push(helpers.formatPiePercent(pt.v / cd0.vTotal, separators));

var hoverLabel = trace.hoverlabel;
var hoverFont = hoverLabel.font;

Fx.loneHover({
x0: hoverCenterX - rInscribed * cd0.r,
x1: hoverCenterX + rInscribed * cd0.r,
y: hoverCenterY,
text: thisText.join('<br>'),
name: hoverinfo.indexOf('name') !== -1 ? trace2.name : undefined,
idealAlign: pt.pxmid[0] < 0 ? 'left' : 'right',
color: helpers.castOption(hoverLabel.bgcolor, pt.pts) || pt.color,
borderColor: helpers.castOption(hoverLabel.bordercolor, pt.pts),
fontFamily: helpers.castOption(hoverFont.family, pt.pts),
fontSize: helpers.castOption(hoverFont.size, pt.pts),
fontColor: helpers.castOption(hoverFont.color, pt.pts)
}, {
container: fullLayout2._hoverlayer.node(),
outerContainer: fullLayout2._paper.node(),
gd: gd
});

Fx.hover(gd, evt, 'pie');

hasHoverData = true;
gd.emit('plotly_hover', {
points: [eventData(pt, trace2)],
event: d3.event
});
hasHoverEvent = true;
}

function handleMouseOut(evt) {
evt.originalEvent = d3.event;
gd.emit('plotly_unhover', {
event: d3.event,
points: [evt]
});
var fullLayout2 = gd._fullLayout;
var trace2 = gd._fullData[trace.index];

if(hasHoverEvent) {
evt.originalEvent = d3.event;
gd.emit('plotly_unhover', {
points: [eventData(pt, trace2)],
event: d3.event
});
hasHoverEvent = false;
}

if(hasHoverData) {
Fx.loneUnhover(fullLayout._hoverlayer.node());
hasHoverData = false;
if(hasHoverLabel) {
Fx.loneUnhover(fullLayout2._hoverlayer.node());
hasHoverLabel = false;
}
}

function handleClick() {
gd._hoverdata = [pt];
gd._hoverdata.trace = cd0.trace;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably didn't mean to attach trace as a property of the whole array 🙈

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. Brutal. Good catch ⚾

var fullLayout2 = gd._fullLayout;
var trace2 = gd._fullData[trace.index];

if(gd._dragging || fullLayout2.hovermode === false) return;

gd._hoverdata = [eventData(pt, trace2)];
Fx.click(gd, d3.event);
}

Expand Down
Loading