Skip to content

Add ability to rename grouped traces #1919

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

Merged
merged 19 commits into from
Aug 15, 2017
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
45 changes: 38 additions & 7 deletions src/components/legend/draw.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ var anchorUtils = require('./anchor_utils');

var SHOWISOLATETIP = true;
var DBLCLICKDELAY = interactConstants.DBLCLICKDELAY;
var BLANK_STRING_REGEX = /^[\s\r]*$/;

module.exports = function draw(gd) {
var fullLayout = gd._fullLayout;
Expand Down Expand Up @@ -392,24 +393,54 @@ function drawTexts(g, gd) {
this.text(text)
.call(textLayout);

var origText = text;

if(!this.text()) text = ' \u0020\u0020 ';

var fullInput = legendItem.trace._fullInput || {},
astr;
var transforms, direction;
var fullInput = legendItem.trace._fullInput || {};
var needsRedraw = false;
var update = {};

// N.B. this block isn't super clean,
// is unfortunately untested at the moment,
// and only works for for 'ohlc' and 'candlestick',
// but should be generalized for other one-to-many transforms
if(['ohlc', 'candlestick'].indexOf(fullInput.type) !== -1) {
var transforms = legendItem.trace.transforms,
direction = transforms[transforms.length - 1].direction;
transforms = legendItem.trace.transforms;
direction = transforms[transforms.length - 1].direction;

update[direction + '.name'] = text;
} else if(Registry.hasTransform(fullInput, 'groupby')) {
var groupbyIndices = Registry.getTransformIndices(fullInput, 'groupby');
var index = groupbyIndices[groupbyIndices.length - 1];

var carr = Lib.keyedContainer(fullInput, 'transforms[' + index + '].groupnames', 'group', 'name');

if(BLANK_STRING_REGEX.test(origText)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, there is a use case for a blank name - maybe less so for groupby than for regular traces, but sometimes you want the title for one trace to stand in for a few traces below it. You could argue then that this should just be testing for an actual empty string '' but a space ' ' should be allowed as the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I could just check === '' instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

carr.remove(legendItem.trace._group);
needsRedraw = true;
} else {
carr.set(legendItem.trace._group, text);
}

update = carr.constructUpdate();
} else {
update.name = text;
}

var p = Plotly.restyle(gd, update, traceIndex);

astr = direction + '.name';
// If a groupby label is deleted, it seems like we need another redraw in order
// to restore the label. Otherwise it simply sets this property and the blank
// string is retained.
if(needsRedraw) {
p = p.then(function() {
return Plotly.redraw(gd);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. We shouldn't have to do this. We should fix _restyle so that it leads to the correct update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'll try to make that happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm having trouble reproducing that this was ever necessary. It seems to update just fine. Giving this the 🔪 and I think it's fine.

});
}
else astr = 'name';

Plotly.restyle(gd, astr, text, traceIndex);
return p;
});
}
else text.call(textLayout);
Expand Down
1 change: 1 addition & 0 deletions src/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ var BADNUM = numConstants.BADNUM;
var lib = module.exports = {};

lib.nestedProperty = require('./nested_property');
lib.keyedContainer = require('./keyed_container');
lib.isPlainObject = require('./is_plain_object');
lib.isArray = require('./is_array');
lib.mod = require('./mod');
Expand Down
116 changes: 116 additions & 0 deletions src/lib/keyed_container.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/**
* 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 nestedProperty = require('./nested_property');

// bitmask for deciding what's updated:
var NONE = 0;
var NAME = 1;
var VALUE = 2;
var BOTH = 3;

module.exports = function keyedContainer(baseObj, path, keyName, valueName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I like it.

@rreusser can you of other situations where this could be useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. It's also used in group styles, for one, though that case might be slightly more complicated. Also frames, though again, slightly more complicated. It's more a matter of realizing that unless we allow custom keys in the schema, abstracting this pattern seems like the only sane way forward.


keyName = keyName || 'name';
valueName = valueName || 'value';
var i, arr;
var changeTypes = {};

if(path && path.length) { arr = nestedProperty(baseObj, path).get();
} else {
arr = baseObj;
}

path = path || '';
arr = arr || [];

// Construct an index:
var indexLookup = {};
for(i = 0; i < arr.length; i++) {
indexLookup[arr[i][keyName]] = i;
}

var obj = {
// NB: this does not actually modify the baseObj
set: function(name, value) {
var changeType = NONE;
var idx = indexLookup[name];
if(idx === undefined) {
changeType = BOTH;
idx = arr.length;
indexLookup[name] = idx;
} else if(value !== arr[idx][valueName]) {
changeType = VALUE;
}
var newValue = {};
newValue[keyName] = name;
newValue[valueName] = value;
arr[idx] = newValue;

changeTypes[idx] = changeTypes[idx] | changeType;

return obj;
},
get: function(name) {
var idx = indexLookup[name];
return idx === undefined ? undefined : arr[idx][valueName];
},
rename: function(name, newName) {
var idx = indexLookup[name];

if(idx === undefined) return obj;
changeTypes[idx] = changeTypes[idx] | NAME;

indexLookup[newName] = idx;
delete indexLookup[name];

arr[idx][keyName] = newName;

return obj;
},
remove: function(name) {
var idx = indexLookup[name];
if(idx === undefined) return obj;
for(i = idx; i < arr.length; i++) {
changeTypes[i] = changeTypes[i] | BOTH;
}
for(i = idx; i < arr.length; i++) {
indexLookup[arr[i][keyName]]--;
}
arr.splice(idx, 1);
delete(indexLookup[name]);

return obj;
},
constructUpdate: function() {
var astr, idx;
var update = {};
var changed = Object.keys(changeTypes);
for(var i = 0; i < changed.length; i++) {
idx = changed[i];
astr = path + '[' + idx + ']';
if(arr[idx]) {
if(changeTypes[idx] & NAME) {
update[astr + '.' + keyName] = arr[idx][keyName];
}
if(changeTypes[idx] & VALUE) {
update[astr + '.' + valueName] = arr[idx][valueName];
}
} else {
update[astr] = null;
}
}

return update;
}
};

return obj;
};
17 changes: 11 additions & 6 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,7 @@ plots.supplyDataDefaults = function(dataIn, dataOut, layout, fullLayout) {

for(i = 0; i < dataIn.length; i++) {
trace = dataIn[i];
fullTrace = plots.supplyTraceDefaults(trace, cnt, fullLayout, i);
fullTrace = plots.supplyTraceDefaults(trace, cnt, fullLayout, i, dataIn.length);
Copy link
Contributor

@etpinard etpinard Jul 28, 2017

Choose a reason for hiding this comment

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

I would prefer stashing dataIn.length on fullLayout then passing it around like this.

Something like fullLayout._dataInLength that could be set here side-by-side to fullLayout._dataLength.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, actually layout._dataLength is the length of the input data that I'm looking for. I think it's best not to make the count depend upon visibility (legend shouldn't fundamentally change when you toggle visibility, right?), so I think it's best to just use layout._dataLength for this.


fullTrace.index = i;
fullTrace._input = trace;
Expand All @@ -809,7 +809,12 @@ plots.supplyDataDefaults = function(dataIn, dataOut, layout, fullLayout) {

for(var j = 0; j < expandedTraces.length; j++) {
var expandedTrace = expandedTraces[j];
var fullExpandedTrace = plots.supplyTraceDefaults(expandedTrace, cnt, fullLayout, i);
var fullExpandedTrace = plots.supplyTraceDefaults(expandedTrace, cnt, fullLayout, i, dataIn.length);

// The group key gets cleared. If set, pass it forward
if(expandedTrace._group) {
fullExpandedTrace._group = expandedTrace._group;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. That's too bad. Do we really need this _group, i.e. could just recompute it during Legend.draw?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe we could call relinkPrivateKeys here, to make this solution generalizable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I didn't like it. I'll try to clean it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to relinkPrivateKeys(fullExpandedTrace, expandedTrace), which seems to me like probably the most general approach here as long as it's not too expensive.

}

// mutate uid here using parent uid and expanded index
// to promote consistency between update calls
Expand Down Expand Up @@ -940,7 +945,7 @@ plots.supplyFrameDefaults = function(frameIn) {
return frameOut;
};

plots.supplyTraceDefaults = function(traceIn, traceOutIndex, layout, traceInIndex) {
plots.supplyTraceDefaults = function(traceIn, traceOutIndex, layout, traceInIndex, inputTraceCount) {
var traceOut = {},
defaultColor = Color.defaults[traceOutIndex % Color.defaults.length];

Expand Down Expand Up @@ -1013,13 +1018,13 @@ plots.supplyTraceDefaults = function(traceIn, traceOutIndex, layout, traceInInde
traceOut.visible = !!traceOut.visible;
}

plots.supplyTransformDefaults(traceIn, traceOut, layout);
plots.supplyTransformDefaults(traceIn, traceOut, layout, inputTraceCount);
}

return traceOut;
};

plots.supplyTransformDefaults = function(traceIn, traceOut, layout) {
plots.supplyTransformDefaults = function(traceIn, traceOut, layout, inputTraceCount) {
var globalTransforms = layout._globalTransforms || [];
var transformModules = layout._transformModules || [];

Expand All @@ -1038,7 +1043,7 @@ plots.supplyTransformDefaults = function(traceIn, traceOut, layout) {
if(!_module) Lib.warn('Unrecognized transform type ' + type + '.');

if(_module && _module.supplyDefaults) {
transformOut = _module.supplyDefaults(transformIn, traceOut, layout, traceIn);
transformOut = _module.supplyDefaults(transformIn, traceOut, layout, traceIn, inputTraceCount);
transformOut.type = type;
transformOut._module = _module;

Expand Down
42 changes: 42 additions & 0 deletions src/registry.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,48 @@ exports.traceIs = function(traceType, category) {
return !!_module.categories[category];
};

/**
* Determine if this trace has a transform of the given type and return
* array of matching indices.
*
* @param {object} data
* a trace object (member of data or fullData)
* @param {string} type
* type of trace to test
* @return {array}
* array of matching indices. If none found, returns []
*/
exports.getTransformIndices = function(data, type) {
var indices = [];
var transforms = data.transforms || [];
for(var i = 0; i < transforms.length; i++) {
if(transforms[i].type === type) {
indices.push(i);
}
}
return indices;
};

/**
* Determine if this trace has a transform of the given type
*
* @param {object} data
* a trace object (member of data or fullData)
* @param {string} type
* type of trace to test
* @return {boolean}
*/
exports.hasTransform = function(data, type) {
var transforms = data.transforms || [];
for(var i = 0; i < transforms.length; i++) {
if(transforms[i].type === type) {
return true;
}
}
return false;

};

/**
* Retrieve component module method. Falls back on noop if either the
* module or the method is missing, so the result can always be safely called
Expand Down
Loading