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.11% 89.12% +0.01%
==========================================
Files 341 341
Lines 73202 73299 +97
==========================================
+ Hits 65231 65328 +97
Misses 7971 7971 ☔ 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.
| # 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.
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.
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.
| 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.
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)