Skip to content

FIX: Handle multiple plots in LegendHighlighter#403

Merged
jwiggins merged 1 commit into
masterfrom
fix/legend-highlighter-multiplot
Mar 30, 2018
Merged

FIX: Handle multiple plots in LegendHighlighter#403
jwiggins merged 1 commit into
masterfrom
fix/legend-highlighter-multiplot

Conversation

@jwiggins
Copy link
Copy Markdown
Member

A Legend can have multiple plot renderers per name.

renderers as values, or lists of renderers.
This function helps us assume we're always working with lists
"""
return obj if isinstance(obj, list) else [obj]
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I briefly looked into the code for Legend to see if it made more sense to fix things there, but that did not appear to be the safe approach.

Comment thread chaco/tools/legend_highlighter.py Outdated
def _reset_selects(self, plots):
""" Set all renderers to their default values. """
for plot in sm.reduce(operator.add, plots.values()):
for plot in sum((_ensure_list(p) for p in plots.values()), []):
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

plots is the dictionary of the same name on the Legend instance. We need to anticipate values() being single renderers or lists of renderers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A better construct for concatenating list-likes is itertools.chain.from_iterable to the point where I sometimes alias that as concat.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup. Done.

@jwiggins
Copy link
Copy Markdown
Member Author

@corranwebster The example code only covers the single plot case. I have a very similar change in some product code if you'd like to see something which exercises the multi-plot case.

Copy link
Copy Markdown
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

Basically OK - I would like to say that we should fix Legend but that would break lots of demo code and therefore by implication lots of user code that copies the pattern.

One nit on how to concatenate sequences.

Comment thread chaco/tools/legend_highlighter.py Outdated
def _reset_selects(self, plots):
""" Set all renderers to their default values. """
for plot in sm.reduce(operator.add, plots.values()):
for plot in sum((_ensure_list(p) for p in plots.values()), []):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A better construct for concatenating list-likes is itertools.chain.from_iterable to the point where I sometimes alias that as concat.

A Legend can have multiple plot renderers per name
@jwiggins jwiggins force-pushed the fix/legend-highlighter-multiplot branch from 120737a to 50f607f Compare March 30, 2018 11:28
@jwiggins
Copy link
Copy Markdown
Member Author

Thanks for the feedback!

@jwiggins jwiggins merged commit 9429955 into master Mar 30, 2018
@jwiggins jwiggins deleted the fix/legend-highlighter-multiplot branch March 30, 2018 15:48
@jonathanrocher
Copy link
Copy Markdown
Collaborator

Want to make sure I understand the fix. Before merging, selecting a name would show and hide only 1 of the renderers, am I guessing correctly? Mind adding details in the description of the issue?

@jwiggins
Copy link
Copy Markdown
Member Author

@jonathanrocher That's correct.

If you have a plot with a legend where, for example, you have a single logical curve/plot which is broken into multiple segments, and thus multiple renderers, the Legend class can handle this. However, until now the LegendHighlighter ignored this feature and just highlighted the first renderer returned from the plots dictionary. Additionally, if you didn't treat Legend.plots as a dictionary of lists of renderers (like much of the demo code), then an exception would be raised by most methods in LegendHighlighter. Both of those issues were corrected here.

jwiggins added a commit that referenced this pull request Jul 12, 2018
FIX: Handle multiple plots in LegendHighlighter
@jwiggins jwiggins mentioned this pull request Jul 12, 2018
@kitchoi kitchoi mentioned this pull request Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants