-
-
Notifications
You must be signed in to change notification settings - Fork 407
fix: add tools directly to Overlay #6713
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
base: main
Are you sure you want to change the base?
Conversation
|
This works now but it seems a bit verbose to me. Will try to find a more elegant way to implement... |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6713 +/- ##
==========================================
+ Coverage 89.21% 89.22% +0.01%
==========================================
Files 332 332
Lines 72027 72120 +93
==========================================
+ Hits 64259 64352 +93
Misses 7768 7768 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Have pushed some changes. Left some todos and info with your name directly in the code. One thing I noticed is that the tool ordering seems weird; it would be nice to have more consistency. I also think we should check that this fixes #6654. |
|
|
||
| tool_types = [type(tool).__name__ for tool in plot.state.tools] | ||
| defaults = ['WheelZoomTool', 'SaveTool', 'PanTool', 'BoxZoomTool', 'ResetTool'] | ||
| # INFO(Azaya): Can this really be right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # INFO(Azaya): Can this really be right? |
Hmm, I found this interesting...
The test passes because it validates the internal tool ordering (i.e, checking plot.state.tools) but visually when you actually plot the same code, the ordering is now different (['PanTool', 'BoxZoomTool', 'WheelZoomTool', 'ZoomOutTool', 'ZoomInTool', 'SaveTool', 'ResetTool']). Maybe this is a result of how Bokeh handles the visual arrangement of the tools?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why this is the case at the top of my head. Try to see if you can understand by looking at the code and trying to use pure bokeh examples. Maybe also related to merge_tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's definitely from bokeh
from bokeh.plotting import figure, show
from bokeh.io import output_notebook
output_notebook()
x = [1, 2, 3, 4, 5]
y = [4, 5, 5, 7, 2]
default_tools = ['wheel_zoom', 'save', 'pan', 'box_zoom', 'reset']
extra_tools = ['zoom_out', 'zoom_in']
# create a plot
p = figure(
title="Toolbars",
tools=extra_tools+default_tools,
width=400,
height=300,
)
p.line(x, y)
show(p)
p.toolbar.tools[ZoomOutTool(id='p1475', ...),
ZoomInTool(id='p1476', ...),
WheelZoomTool(id='p1477', ...),
SaveTool(id='p1478', ...),
PanTool(id='p1479', ...),
BoxZoomTool(id='p1480', ...),
ResetTool(id='p1488', ...)]
So while plot.toolbar.tools respects the set order of the tools, visually it still follows the other way (i.e, ['PanTool', 'BoxZoomTool', 'WheelZoomTool', 'ZoomOutTool', 'ZoomInTool', 'SaveTool', 'ResetTool'])
overlay_plot.mov |
holoviews/plotting/bokeh/util.py
Outdated
| for name in ("dimensions", "tags", "name", "description", "icon"): | ||
| if identifier := getattr(tool, name, None): | ||
| # Convert lists/tuples to tuples (hashable) | ||
| if isinstance(identifier, list | tuple): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if isinstance(identifier, list | tuple): | |
| if isinstance(identifier, list): |
Don't use pipe for isinstance and no need to check for tuple.
holoviews/plotting/bokeh/util.py
Outdated
| for name in ("dimensions", "tags", "name", "description", "icon"): | ||
| if identifier := getattr(tool, name, None): | ||
| # Convert lists to tuples (hashable) | ||
| if isinstance(identifier, list): | ||
| identifier = tuple(identifier) | ||
| return tool_type, identifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the name is the same but the icon is not? They should likely not be identified as the same thing.

fixes #6240
fixes #6654
This PR fixes the problem where tools specified via
.opts(tools=[...])on an Overlay were being ignored. The OverlayPlot._init_tools() method only collected tools from subplots but never processed tools fromself.default_toolsandself.tools.Solution
self.default_tools + self.toolsand add them to the plottool_typeslist) with unique identifier-based deduplication (tool_idsset). This is better since most bokeh tools like PanTool have variants differentiated only by their dimensions (e.g., 'pan', 'xpan', 'ypan' are all of type PanTool)