Skip to content

Conversation

@Azaya89
Copy link
Contributor

@Azaya89 Azaya89 commented Oct 28, 2025

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 from self.default_tools and self.tools.

Solution

  • Added logic to iterate through self.default_tools + self.tools and add them to the plot
  • Improved tool deduplication: Replaced simple type-based deduplication (tool_types list) with unique identifier-based deduplication (tool_ids set). 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)
  • Preserved special handling for hover tools and subcoordinate_y zoom tools which was causing the initial test failures
  • Added tests
import holoviews as hv
hv.extension('bokeh')

overlay = (hv.Curve([1, 2, 3]).opts(tools=['xpan']) * hv.Curve([2, 3, 4]).opts(tools=['ypan', 'zoom_out'])).opts(tools=['zoom_in', 'xpan', 'ypan'])
overlay
Screenshot 2025-11-27 at 7 33 49 PM

@Azaya89 Azaya89 self-assigned this Oct 28, 2025
@Azaya89
Copy link
Contributor Author

Azaya89 commented Oct 28, 2025

This works now but it seems a bit verbose to me. Will try to find a more elegant way to implement...

@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.22%. Comparing base (47f696f) to head (cdfbfe3).
⚠️ Report is 1 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Azaya89 Azaya89 marked this pull request as ready for review October 29, 2025 15:11
@Azaya89 Azaya89 requested review from hoxbro and maximlt October 29, 2025 15:11
@hoxbro hoxbro added this to the 1.22.1 milestone Oct 31, 2025
@hoxbro
Copy link
Member

hoxbro commented Nov 28, 2025

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?
Copy link
Contributor Author

@Azaya89 Azaya89 Nov 28, 2025

Choose a reason for hiding this comment

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

Suggested change
# 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?

Copy link
Member

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.

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 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)
Screenshot 2025-12-02 at 9 43 14 AM
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'])

@Azaya89
Copy link
Contributor Author

Azaya89 commented Nov 28, 2025

overlay_plot.mov

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):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if isinstance(identifier, list | tuple):
if isinstance(identifier, list):

Don't use pipe for isinstance and no need to check for tuple.

@hoxbro hoxbro modified the milestones: 1.22.1, 1.22.2 Dec 1, 2025
Comment on lines 1282 to 1287
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
Copy link
Member

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.

@Azaya89
Copy link
Contributor Author

Azaya89 commented Dec 4, 2025

What happens if the name is the same but the icon is not? They should likely not be identified as the same thing.

I've added implementation to identify as different types if there are different disambiguation properties. This now means that something like this will produce multiple PanTool instances:

import holoviews as hv
hv.extension('bokeh')

hv.Curve([1, 2, 3]).opts(tools=[PanTool(description='New Pan')])
Screenshot 2025-12-04 at 11 50 20 AM

@Azaya89 Azaya89 requested a review from hoxbro December 5, 2025 12:30
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.

Overlay does not have separate zoom controls by axis Adding tools to an Overlay has no effect

3 participants