Allow to access tools via a context menu by mattpap · Pull Request #14228 · bokeh/bokeh

mattpap · 2025-01-15T01:38:42Z

This PR adds support for automatically generated menu that mimics plot's toolbar. This way tools are accessible even when Toolbar isn't visible.

fixes #14225

mattpap · 2025-01-16T00:38:09Z

It is possible to configure the ToolMenu as a sub-menu of another menu, as showed in examples/interaction/tools/tool_context_menu:

p.context_menu = Menu(
    items=[
        MenuItem(label="Do something", action=CustomJS(code="alert('did something')")),
        None,
        MenuItem(label="Tools", menu=ToolMenu(toolbar=p.toolbar)),
    ],
)

mattpap · 2025-01-16T16:22:53Z

~~This is tentatively ready for review. Some testing is needed to finish this PR~~.

bryevdv · 2025-01-23T04:11:20Z

+    of an item. Unchecked item is represented with an empty space.
+
+    The menu will allocate a column for check marks for all its items if
+    at least one item has set a boolean value for ``checked`` property.


This seems a little awkward. If I want a list of checkable, but all initially unchecked items, I need to either:

pick one at random to set checked=False (arbitrary, inconsistent)

explicitly set all checked=False (a bit tedious, False is a reasonable default to assume for checked)

?

Alternatives are:

Leave CheckableItem undeprecated and override checked default value with False.

Add Menu.checkable: bool | "auto", where "auto" is the default current behavior. False seems not particlarly useful though.

I'm not sure I like any of these. It just seems to me that "checkability" is an explicit up-front decision, so there's no need for all the magic on the base class (i.e. ActionItem should just not have checked at all). But if you don't like that route for some reason I suppose the way things is preferable over the two options above.

Given that non of these properties are mutually exclusive, I figured that only having MenuItem will be preferable, instead of more complex complex model hierarchy, and easier for the user if they change their mind (no need to change the model and imports). Other frameworks that provide context menus (e.g. Qt, WinForms) do the same.

I also don't like this when I put myself in the shoes of someone implementing a menu with this library. In my view, having a nullable boolean variable named checked creates cognitive overhead. From what I could tell, the other frameworks you cited, Qt and Windows Forms, do not try to stuff the semantics of "is checkable" (in your code, maps to null / not null) and "is checked" (true / false) into a single variable. Same goes for HTML forms: <input type="checkbox" checked />.

I guess I don't understand why Bryan doesn't like option 2, Menu.checkable, nor do I understand why it should have a type other than a simple boolean whose default value is false. That way, at the end of the day, you have two separate booleans to express three possible states (i.e.: not checkable; checkable and checked; checkable and unchecked) rather than one nullable boolean to express those same three states.

Continuation:

Checkable menu item API #14338

gabalafou

I really like the direction this code is taking. I am quite a bit late to the party but I spent a lot of time reading and poking at this code, so I thought I should share my review even though the pull request has already been merged. A lot of my remarks are just questions to help me better understand the codebase.

Most of my comments are inline, but I have two UX remarks that I will leave here:

I don't think that native context menus take an action or close when you click on a menu item that has a sub-menu. That means the current behavior in Bokeh may be an anti-pattern. I don't think this PR introduced that behavior. But I thought it was still worth noting.
I also noticed that for sub-menus of certain tools when you select one of the sub-tools, the icon for that tool is not updated in the context menu, which I think is a bit confusing. For example, the pan tool has three states: X, Y, and both X and Y. If you choose the Y state, the icon in the context menu does not update to reflect that it is now in the Y state. Again, I don't think this is something that this pull request changed, but putting it down so it's recorded somewhere.

gabalafou · 2025-01-27T23:30:30Z

+
+      const {view} = await display(p)
+
+      // can't simply dispatchEvent() because if browser security


Suggested change

// can't simply dispatchEvent() because if browser security

// can't simply dispatchEvent() because of browser security

gabalafou · 2025-01-28T13:18:06Z

@@ -0,0 +1,49 @@
+""" Example showing how to configure the context menu of a plot, including


Should the context menu have highest z-index?

I don't think so. You may have a tooltip for an element of a menu. In this particular case, the top level component should be the one showed last.

gabalafou · 2025-01-28T14:26:35Z

  target: HTMLElement
  orientation?: Orientation
  reversed?: boolean
+  labels?: boolean


The variable name labels makes me think it is an array of strings, not a boolean.

Sure, show_labels would have been better, but this will be removed soon-ish anyway.

gabalafou · 2025-01-28T19:12:48Z

+      }),
+      new MenuItem({
+        icon: `.${icons.tool_icon_auto_box_zoom}`,
+        label: "Auto mode",


Out of curiosity, how does auto mode work? How exactly does it depend on the mouse gesture? For me it seems to behave exactly like XY mode, and neither of the following docs pages clarifies it for me:

https://docs.bokeh.org/en/dev-3.7/docs/user_guide/interaction/tools.html#boxzoomtool

https://docs.bokeh.org/en/dev-3.7/docs/reference/models/tools.html#bokeh.models.BoxZoomTool

Update: I figured it out. It behaves like X-only mode if the user drags the mouse along a horizontal straight line in the x direction, like Y-only mode if the user drags only in the y direction (i.e., along a vertical line), and like XY mode if the user drags the mouse in the both the x and y directions (neither a strictly horizontal nor strictly vertical line). Making a note to see if this is clearly conveyed in the docs somewhere.

It may not be in the case of this particular tool, but that kind of behavior should be documented for other tools that implement it as well. Still it should be documented regardless.

gabalafou · 2025-01-29T09:15:40Z

-    const menus = this.model.items
-      .map((item) => item instanceof ActionItem ? item.menu : null)
+    const menus = this.menu_items
+      .map((item) => item instanceof MenuItem ? item.menu : null)


The way this was written is peculiar... wouldn't you agree that it's a bit more natural to filter first then map?

const menus = this.menu_items .filter((item) => item instanceof MenuItem) .map((item) => item.menu)

Peculiar indeed. Maybe a side effect of some earlier code.

Actually, you need another filter because item.menu may be null. Also, I'm still not used to TypeScript properly narrowing the return type in filter() with instanceof checks (fairly recent addition to the type system).

gabalafou · 2025-01-31T18:14:34Z

+    of an item. Unchecked item is represented with an empty space.
+
+    The menu will allocate a column for check marks for all its items if
+    at least one item has set a boolean value for ``checked`` property.


I also don't like this when I put myself in the shoes of someone implementing a menu with this library. In my view, having a nullable boolean variable named checked creates cognitive overhead. From what I could tell, the other frameworks you cited, Qt and Windows Forms, do not try to stuff the semantics of "is checkable" (in your code, maps to null / not null) and "is checked" (true / false) into a single variable. Same goes for HTML forms: <input type="checkbox" checked />.

I guess I don't understand why Bryan doesn't like option 2, Menu.checkable, nor do I understand why it should have a type other than a simple boolean whose default value is false. That way, at the end of the day, you have two separate booleans to express three possible states (i.e.: not checkable; checkable and checked; checkable and unchecked) rather than one nullable boolean to express those same three states.

gabalafou · 2025-02-01T01:16:50Z

I understand most of the changes you made to this test file, but I actually don't understand the test file itself. First of all, the name is confusing to me. If I understand the test code correctly, it would make more sense if the file were named annotation_context_menu.ts. Secondly, it looks like there's all of this code that creates a context menu, but then doesn't actually take the necessary steps to open that context menu before taking a snapshot, so essentially it's just snapshotting a kind of scatter plot, doesn't seem to have anything to do with annotations or context menus, since neither of those are depicted in the snapshot:

This is an example more than it is a test, so although image is being captured, in this and some other cases it has secondary importance if any. These examples are a part of integration tests for convenience (at least for now). I need to think about this, because we could set them up like tests, or maybe introduce a modal setup and allow both (under some command like/URL argument option).

mattpap · 2025-02-03T22:06:47Z

@gabalafou, can you start a discussion for checkable item behavior and API? We can still refine this before 3.7.

* Remove unnecessary leftover code * Fix spelling * Use less convoluted logic * Add unit tests

github-actions · 2025-06-04T15:12:06Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mattpap added the status: WIP label Jan 15, 2025

mattpap added this to the 3.7 milestone Jan 15, 2025

mattpap force-pushed the mattpap/14225_tools_context_menu branch 2 times, most recently from 7197eb6 to a5c8270 Compare January 16, 2025 00:36

mattpap force-pushed the mattpap/14225_tools_context_menu branch from a5c8270 to 0ded33b Compare January 16, 2025 16:22

mattpap added status: ready and removed status: WIP labels Jan 16, 2025

mattpap requested a review from hoxbro January 16, 2025 16:42

mattpap force-pushed the mattpap/14225_tools_context_menu branch 2 times, most recently from 76cd99a to b31e214 Compare January 17, 2025 18:41

mattpap mentioned this pull request Jan 19, 2025

Define the initial set of common CSS variables #14177

Closed

bryevdv reviewed Jan 23, 2025

View reviewed changes

mattpap mentioned this pull request Jan 23, 2025

Add support keyboard navigation in menus #14241

Open

mattpap force-pushed the mattpap/14225_tools_context_menu branch from 48d7edd to dfdafcc Compare January 23, 2025 05:38

hoxbro reviewed Jan 24, 2025

View reviewed changes

mattpap mentioned this pull request Jan 24, 2025

Make sure that menus respect viewport/container boundaries #14245

Open

mattpap force-pushed the mattpap/14225_tools_context_menu branch 2 times, most recently from 620edb9 to c257e2d Compare January 25, 2025 22:36

mattpap added 10 commits January 30, 2025 01:15

Allow to access tools via a context menu

0589a63

Simplify menu item related model hierarchy

831d6e3

Add release notes

c3deb18

Update cross integration tests

0ab3495

Ignore deprecation warnings in constructibility tests

09a69fe

Allow None/null in place of DividerItem

ea9b6d3

Migrate tools to using MenuItem model

0e93dd6

Improve disabled menu item cursor

aef24dd

Add integration tests

355f486

Prepare for key binding handling

ad1f481

mattpap added 8 commits January 30, 2025 01:15

Extract preliminary CSS variable API

2d52320

Make the example more interactive

7952168

Don't cancel native menu if custom menu didn't show

2eee529

Improve usage of range() iterator

172e58f

Add support for UIElement.context_menu == "auto"

4f0fef5

Rename {menus->legacy_menus}.less and menus{_->}.less

87a3299

Don't display icon column when not using icons

2c8cd35

Update type declarations

db5254b

mattpap force-pushed the mattpap/14225_tools_context_menu branch from c257e2d to db5254b Compare January 30, 2025 00:23

hoxbro approved these changes Jan 30, 2025

View reviewed changes

mattpap added status: accepted and removed status: ready labels Jan 30, 2025

mattpap merged commit 19ba6ca into branch-3.7 Jan 30, 2025

mattpap deleted the mattpap/14225_tools_context_menu branch January 30, 2025 08:56

gabalafou restored the mattpap/14225_tools_context_menu branch January 30, 2025 21:56

gabalafou reviewed Feb 3, 2025

View reviewed changes

gabalafou deleted the mattpap/14225_tools_context_menu branch February 3, 2025 21:06

gabalafou mentioned this pull request Feb 14, 2025

Address late review of PR #14228 #14276

Closed

mattpap added a commit that referenced this pull request Feb 14, 2025

Address late review comments from PR #14228 (#14277)

0e0c5d8

* Remove unnecessary leftover code * Fix spelling * Use less convoluted logic * Add unit tests

mattpap mentioned this pull request Feb 15, 2025

remove unnecessary tools in legends example #14332

Merged

1 task

RamiDarwiche pushed a commit to RamiDarwiche/bokeh that referenced this pull request Feb 23, 2025

Address late review comments from PR bokeh#14228 (bokeh#14277)

e53dd9c

* Remove unnecessary leftover code * Fix spelling * Use less convoluted logic * Add unit tests

github-actions Bot locked as resolved and limited conversation to collaborators Jun 4, 2025


		const {view} = await display(p)

		// can't simply dispatchEvent() because if browser security

		@@ -0,0 +1,49 @@
		""" Example showing how to configure the context menu of a plot, including

Uh oh!

Uh oh!

Conversation

mattpap commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattpap commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattpap commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryevdv Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabalafou Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gabalafou left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattpap Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattpap Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattpap commented Jan 15, 2025 •

edited

Loading

mattpap commented Jan 16, 2025 •

edited

Loading

mattpap commented Jan 16, 2025 •

edited

Loading

bryevdv Jan 23, 2025 •

edited

Loading

gabalafou Feb 17, 2025 •

edited

Loading

gabalafou left a comment •

edited

Loading

mattpap Feb 3, 2025 •

edited

Loading

mattpap Feb 3, 2025 •

edited

Loading