Order issues when appending to permuted legends
Closed this issue · 4 comments
legtools.append operates under the assumption that the order of the legend entries is always the same as the order of the plot objects in the parent axes. Because of this, if we permute the legend entries and then append/remove entries, the legend entries can become inconsistent with the plot entries.
For example:
% Set up figure
x = 1:10;
y1 = x;
y2 = x + 1;
figure
plot(x, y1, x, y2);
lh = legend('y1', 'y2');
legtools.adddummy(lh, 'dummy', 'ok')
legtools.permute(lh, [3 2 1])
legtools.remove(lh, 2)
legtools.append(lh, 'y2') % Now we're off the rails
In this example we can see that the legend order should be [y1, y2, dummy] rather than the displayed [dummy, y1, y2].
See #7
I'd like to try to create a PR for this issue for the experience, while providing only commits for this issue that are as compatible with the current repo state as possible (as discussed in #7). I think the fix I thought for this bug is quite nice (see this commit), because of simplicity:
- Find the indices in the current legend's plot children where the current axes' children (
plothandles) are found (icurrent). We want to keep the future legend's plot children to be in the same order as the current legend's plot children, then append to this current list of entries. - Find the indices of children in
plothandlesthat are not already the current legend's plot children (idiff). - Set the future legend's plot children to become an equivalent of
plothandles([icurrent; idiff]). Possilble issue (requires testing): what happensidiffpoints to more children than are to be appended, an thusnumel([icurrent; idiff]) > numel(newlegendstr)? I expect no errors or warnings, but we could limit the number of future legend's new plot children.
I'd rewrite my original commit with the current release of legtools as my starting point.
This can be handled with union:
lh.PlotChildren = union(lh.PlotChildren, plothandles, 'stable');
Which will append the graphics objects that are children of the parent axes not present in lh's PlotChildren to the end of the object array. This can never have more elements than the number of children in the parent Axes object that are supported by legend. MATLAB throws a warning when attempting to add more legend strings than legend entries; legtools currently doesn't but it will be added so the behavior is consistent and apparent to the user.
Both approaches share an issue when attempting to append multiple entries after the order of legend entries is permuted away from the order of the children of the parent axes. Accurate legend string assignment relies on knowledge of the plot order, which cannot be assumed.
For example:
x = 1:10;
y1 = x;
y2 = x + 1;
y3 = x + 2;
figure
subplot 121
plot(x, y1, x, y2, x, y3);
lh1 = legend('y1', 'y2', 'y3');
legtools.permute(lh1, [3 1 2])
legtools.remove(lh1, [1 2])
legtools.append(lh1, {'y1', 'y3'})
subplot 122
plot(x, y1, x, y2, x, y3);
lh2 = legend('y1', 'y2', 'y3');
legtools.permute(lh2, [3 1 2])
legtools.remove(lh2, [1 2])
legtools.append(lh2, {'y3', 'y1'})
Produces:
One potential avenue for fixing this is to adjust the plot order when calling legtools.permute. I don't want to do this because adjusting the render order can negatively impact a user's visualization. Another hackey solution would be to rely on 'DisplayName' being accurate or modifying some other property of the object, which goes down another rabbit hole of fringe cases for what is, to be honest, already a pretty fringe case.
I don't think your example is a problem, but simply the user appending the wrong names! You use legtools in a way it wasn't written for.
In the case you call incorrect, you simply set the name of the y1 data to 'y3' and the name of the y3 data to 'y1'. Appending to the legend requires the user to know in what order the plot children are. This can be assumed to be true, since often plots are made in a certain order: last plotted is last in the legend. Simply using append will add to the legend based on the plot children order. This is the intended behaviour, not a bug in my eyes. So, if the user wants to remove 'y1' and 'y3' from the legend, then re-add them in a different order, the user should have used permute in the first place instead of remove and append.
The user should be notified in the help section of append about this behaviour: append adds entries to the legend based on the plot children order. First in line (last in ax.Children) not already in the legend is the next entry to be appended with the new label. If this is too much implicit behaviour, a solution would be to require the user to explicitly provide the handles of the plot children to append to the legend. This seems to me too much of a hassle for the user. Relying on 'DisplayName' is not a robust solution.
I agree with you that permute shouldn't mess with the plot children order. It's ‘legend tools’, after all, not ‘plot tools’. The plot should be left untouched (which is already slightly violated by adddummy, but clearly stated in the documentation), so the user knows their plot's properties aren't altered (except for the legend).
Appending to the legend requires the user to know in what order the plot children are. This can be assumed to be true, since often plots are made in a certain order: last plotted is last in the legend.
My point is that this can't be assumed true. Pretend you've appended, permuted, removed, etc. and then saved a *.fig file. A user interacting with just "blank" slate has no knowledge of what value is going to be appended first when finding the union of the legend object's PlotChildren and the Axes object's Children, so appending values properly is a total guess unless they explicitly look at these properties.
So, if the user wants to remove 'y1' and 'y3' from the legend, then re-add them in a different order, the user should have used permute in the first place instead of remove and append.
Yes, it's a contrived example just like the original example was contrived. That doesn't make it less valid.

