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 14 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
32 changes: 25 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,41 @@ 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 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');

astr = direction + '.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);
} else {
carr.set(legendItem.trace._group, text);
}

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

Plotly.restyle(gd, astr, text, traceIndex);
return Plotly.restyle(gd, update, traceIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

fun 👍

});
}
else text.call(textLayout);
Expand Down
31 changes: 31 additions & 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 Expand Up @@ -727,3 +728,33 @@ lib.numSeparate = function(value, separators, separatethousands) {

return x1 + x2;
};

var TEMPLATE_STRING_REGEX = /%{([^\s%{}]*)}/g;
var SIMPLE_PROPERTY_REGEX = /^\w*$/;

/*
* Substitute values from an object into a string
*
* Examples:
* Lib.templateString('name: %{trace}', {trace: 'asdf'}) --> 'name: asdf'
* Lib.templateString('name: %{trace[0].name}', {trace: [{name: 'asdf'}]}) --> 'name: asdf'
*
* @param {string} input string containing %{...} template strings
* @param {obj} data object containing substitution values
*
* @return {string} templated string
*/

lib.templateString = function(string, obj) {
// Not all that useful, but cache nestedProperty instantiation
// just in case it speeds things up *slightly*:
var getterCache = {};

return string.replace(TEMPLATE_STRING_REGEX, function(dummy, key) {
if(SIMPLE_PROPERTY_REGEX.test(key)) {
return obj[key] || '';
}
getterCache[key] = getterCache[key] || lib.nestedProperty(obj, key).get;
return getterCache[key]() || '';
});
};
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;
};
4 changes: 4 additions & 0 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,10 @@ plots.supplyDataDefaults = function(dataIn, dataOut, layout, fullLayout) {
var expandedTrace = expandedTraces[j];
var fullExpandedTrace = plots.supplyTraceDefaults(expandedTrace, cnt, fullLayout, i);

// relink private (i.e. underscore) keys expanded trace to full expanded trace so
// that transform supply-default methods can set _ keys for future use.
relinkPrivateKeys(fullExpandedTrace, expandedTrace);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment here e.g.:

relink private (i.e. underscore) keys expanded trace to full expanded trace so that transform supply-default methods can set _ keys for future use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


// mutate uid here using parent uid and expanded index
// to promote consistency between update calls
expandedTrace.uid = fullExpandedTrace.uid = fullTrace.uid + j;
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
68 changes: 64 additions & 4 deletions src/transforms/groupby.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,35 @@ exports.attributes = {
'with `x` [1, 3] and one trace with `x` [2, 4].'
].join(' ')
},
nameformat: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Bringing in @alexcjohnson @cldougl @chriddyp @cpsievert @monfera what do you think of this syntax?

A lot of existing attributes could benefit form special formatting character like this one. For example,

trace = {
  y: [1, 2, 1]
  marker: {
    size: [20, 30, 10]
  },
  hovertext: '%marker.size'
}

would be a nice shortcut to show each point's marker.size value on hover.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I like %. Though maybe I'd vote for enforcing it with {} e.g. %{name} similar to ES6 ${name}.

Copy link
Contributor

Choose a reason for hiding this comment

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

... and I would make user spell out %{name} and %{group} as opposed to e.g. %t and %g

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 really like the %{group} format.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this is cool! I hadn't thought about extending this to other keys but I can imagine that going off in all sorts of directions in the future (%{marker.size*5}???).

%{...} seems sufficiently useless on its own that we can do this without calling it a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. Like a general substitution framework? Yeah, the goal here was just a one-off custom substitution in which it only knows about a couple properties. But definitely possibilities. The only thing then would be choosing awkward names like %{group} and %{name} (instead of %{trace}) so that it's backward compatibility in case we substitute trace properties instead of special/custom/ad hoc keys.

Copy link
Contributor Author

@rreusser rreusser Aug 1, 2017

Choose a reason for hiding this comment

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

@etpinard @alexcjohnson I've added Lib.templateString that performs these substitutions. At the moment it just grabs object properties if it's simple or uses nestedProperty if the key is more complicated than just a plain word. For this case the only data available is %{trace} and %{group}, but it at least opens the door to a unified interface for taking this further (or worst-case-scenario it's only a couple lines of code if it's not more generally useful). Feedback welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See:

lib.templateString = function(string, obj) {

valType: 'string',
description: [
'Pattern by which grouped traces are named. If only one trace is present,',
'defaults to the group name (`"%{group}"`), otherwise defaults to the group name',
'with trace name (`"%{group} (%{trace})"`). Available escape sequences are `%{group}`, which',
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be %{name} instead of %{trace} so that it corresponds to an attribute just like group?

Copy link
Contributor

Choose a reason for hiding this comment

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

... but wait, group isn't a attribute either (groups is though). So how should we do this?

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, you're right. There are two options here:

  1. Expose special sequences. That's what I've done here. So only group and trace are available. They're custom.
  2. Make this a more general pattern of evaluating trace properties, in which case it would indeed be %{name} and… well, there's not really a property for the group unless we add it custom which is why I went with 1 here.

Copy link
Contributor Author

@rreusser rreusser Aug 2, 2017

Choose a reason for hiding this comment

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

Though to be honest, I'm starting to think we should intercept %{group} and %{name} for this special case but also allow more general evaluation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm starting to think we should intercept %{group} and %{name} for this special case

I'm starting to think the same. 👍 for the current implementation.

Copy link
Contributor Author

@rreusser rreusser Aug 2, 2017

Choose a reason for hiding this comment

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

One small change though, right? Perhaps templateString should accept multiple objects from which to pull values and it takes the first one with a value. Then group and name would be the first preference before grabbing trace properties. Specifically:

Now:

Lib.templateString('%{group} (%{trace})', {group: ..., trace: ...})

Proposed:

Lib.templateString('%{group} (%{name})', {group: ..., name: ...}, traceObj);

So that technically %{marker.size} would be equally valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

This plural vs singular problem came up too in #1770 ➡️ here - where values of plural attributes are listed using a singular (grammatically) key.

Maybe we should start declaring corresponding singular key in the plot schema and have Lib.templateString and appendArrayPointValue use it.

Copy link
Contributor Author

@rreusser rreusser Aug 2, 2017

Choose a reason for hiding this comment

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

Interesting. The only tricky part is that to be really precise, it would have to be %{transforms[2].group}, which it then links up with the schema to realize the pluralized key is in the schema and is a data array, and can be indexed and expanded to %{transforms[2].groups[12]}. The problem I see with that is that templateString must then receive the schema and an item index and that the key must contain the transform number, both of which make it harder to implement and use. Correct me if I'm mistaken though.

'inserts the group name, and `%{trace}`, which inserts the trace name. If grouping',
'GDP data by country when more than one trace is present, for example, the',
'default "%{group} (%{trace})" would return "Monaco (GDP per capita)".'
].join(' ')
},
groupnames: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. I missed this during my first review. I'm not a fan of groupnames being an array container.

We could instead declare groupnames as a data_array and expecting its length to be equal to groups.length, but that would lead to some (maybe a lot of) duplication.

Maybe we could groupname attribute to the styles container below instead which already expects a target group?

Actually, do we even need a groupnames attribute? groups items can be any strings, so setting groupnames[i].name is equivalent to changing the corresponding groups item.

I apologize for not catching this sooner.

Copy link
Collaborator

Choose a reason for hiding this comment

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

setting groupnames[i].name is equivalent to changing the corresponding groups item.

Not quite - the name will be set by nameformat - ie a combination of the group value and the trace name by default - but when you want to override that (perhaps by typing something specific in the editable legend) you'll need this.

We could instead declare groupnames as a data_array and expecting its length to be equal to groups.length, but that would lead to some (maybe a lot of) duplication.

Or ambiguity, if the names were not perfectly duplicated. What would that mean?

Maybe we could groupname attribute to the styles container below instead which already expects a target group?

That sounds great! so each entry in styles would have target and optionally value and/or name? No need to have two separate containers both keyed on the group value.

Copy link
Contributor Author

@rreusser rreusser Aug 7, 2017

Choose a reason for hiding this comment

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

The main problem is that groups have no correspondence with groupnames, which I think rules out a plain array for the API. I think there's a good argument to be made though for merging styles with groupnames. Actually in hindsight, that does seem best to me. Not sure why I didn't realize that sooner…

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds great! so each entry in styles would have target and optionally value and/or name?

Exactly. I'd vote for name over value, but I don't have a strong opinion here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

value is all the other styles... actually, would it work to just put name inside value, and not have to create any new API? We'd still of course have to point the editing routine at styles[i].value.name but seems like that's all, right?

Copy link
Contributor Author

@rreusser rreusser Aug 7, 2017

Choose a reason for hiding this comment

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

I think that's the best option. 👍 Then it really is just a style property (which could be set anyway and in fact we'd be doing something surprising not to use what's already possible).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, IMO, TBH, I'm even happier now that I took the time to implement keyedContainer…

Copy link
Contributor

Choose a reason for hiding this comment

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

would it work to just put name inside value

good call 👍

_isLinkedToArray: 'groupname',
group: {
valType: 'string',
role: 'info',
description: [
'An group to which this name applies. If a `group` and `name` are specified,',
'that name overrides the `nameformat` for that group\'s trace.'
].join(' ')
},
name: {
valType: 'string',
role: 'info',
description: [
'Trace names assigned to the grouped traces of the corresponding `group`.'
].join(' ')
}
},
styles: {
_isLinkedToArray: 'style',
target: {
Expand Down Expand Up @@ -71,7 +100,8 @@ exports.attributes = {
* @return {object} transformOut
* copy of transformIn that contains attribute defaults
*/
exports.supplyDefaults = function(transformIn) {
exports.supplyDefaults = function(transformIn, traceOut, layout) {
var i;
var transformOut = {};

function coerce(attr, dflt) {
Expand All @@ -83,12 +113,24 @@ exports.supplyDefaults = function(transformIn) {
if(!enabled) return transformOut;

coerce('groups');
coerce('nameformat', layout._dataLength > 1 ? '%{group} (%{trace})' : '%{group}');

var nameFormatIn = transformIn.groupnames;
var nameFormatOut = transformOut.groupnames = [];

if(nameFormatIn) {
for(i = 0; i < nameFormatIn.length; i++) {
nameFormatOut[i] = {};
Lib.coerce(nameFormatIn[i], nameFormatOut[i], exports.attributes.groupnames, 'group');
Lib.coerce(nameFormatIn[i], nameFormatOut[i], exports.attributes.groupnames, 'name');
}
}

var styleIn = transformIn.styles;
var styleOut = transformOut.styles = [];

if(styleIn) {
for(var i = 0; i < styleIn.length; i++) {
for(i = 0; i < styleIn.length; i++) {
styleOut[i] = {};
Lib.coerce(styleIn[i], styleOut[i], exports.attributes.styles, 'target');
Lib.coerce(styleIn[i], styleOut[i], exports.attributes.styles, 'value');
Expand Down Expand Up @@ -130,9 +172,9 @@ exports.transform = function(data, state) {
return newData;
};


function transformOne(trace, state) {
var i, j, k, attr, srcArray, groupName, newTrace, transforms, arrayLookup;
var groupNameObj;

var opts = state.transform;
var groups = trace.transforms[state.transformIndex].groups;
Expand All @@ -153,6 +195,10 @@ function transformOne(trace, state) {
styleLookup[styles[i].target] = styles[i].value;
}

if(opts.groupnames) {
groupNameObj = Lib.keyedContainer(opts, 'groupnames', 'group', 'name');
}

// An index to map group name --> expanded trace index
var indexLookup = {};

Expand All @@ -162,7 +208,21 @@ function transformOne(trace, state) {

// Start with a deep extend that just copies array references.
newTrace = newData[i] = Lib.extendDeepNoArrays({}, trace);
newTrace.name = groupName;
newTrace._group = groupName;

var suppliedName = null;
if(groupNameObj) {
suppliedName = groupNameObj.get(groupName);
}

if(suppliedName) {
newTrace.name = suppliedName;
} else {
newTrace.name = Lib.templateString(opts.nameformat, {
trace: trace.name,
group: groupName
});
}

// In order for groups to apply correctly to other transform data (e.g.
// a filter transform), we have to break the connection and clone the
Expand Down
Loading