Context menu 3d #6
Conversation
r3kste
left a comment
r3kste
left a comment
There was a problem hiding this comment.
@AdwaithBatchu The backend implementation is slightly wrong right now. As discussed previously:
- There should be no mention of 3d axes or
view_initwithin the backend files. After all these files should be as general as possible. - You only need to add a function named
context_menuin the FigureManager class.
AdwaithBatchu
force-pushed
the
context-menu-3d
branch
from
9e02876 to
2086384
Compare
There was a problem hiding this comment.
@AdwaithBatchu I have some concerns about implementation in Gtk4 and MacOSX.
Apart from that, I want you to link to the official documentation of the context_menu for each backend. For example, I have added a comment for qt backend. Do this for the other implemented backends.
|
|
||
| if (dx**2 + dy**2) > 5: | ||
| self._mouse_moved = True | ||
|
|
There was a problem hiding this comment.
Isn't this threshold a bit too high?
There was a problem hiding this comment.
This threshold is now not too high, but is still noticeable. If I move the cursor slightly, the plot is zoomed as well as the context menu is shown.
r3kste
left a comment
r3kste
left a comment
There was a problem hiding this comment.
@AdwaithBatchu This is pretty close. The main thing is that
Make sure all backends use a consistent method for determining location
1. Current mouse position
2. Extract coords from event
Both will (should) give the exact same output, but for sake of consistency we should use a single method. I think (ii) is better, but let me know if you think otherwise
lib/mpl_toolkits/mplot3d/axes3d.py
Outdated
| functools.partial(draw_lambda, elev=0, azim=0), | ||
| functools.partial(draw_lambda, elev=0, azim=-90)], | ||
| ) | ||
| canvas.draw_idle() |
There was a problem hiding this comment.
| canvas.draw_idle() |
This can be removed now
| for label, action in zip(labels, actions): | ||
| item = Gtk.MenuItem(label=label) | ||
| menu.append(item) | ||
| item.connect('activate', lambda w, a=action: a()) |
There was a problem hiding this comment.
If a parameter is unused, just use underscore. Do this in other backends that use such lambdas (such as GTK4)
| item.connect('activate', lambda w, a=action: a()) | |
| item.connect('activate', lambda _, a=action: a()) |
| menu = Gio.Menu() | ||
| action_group = Gio.SimpleActionGroup() | ||
| for i, (label, action) in enumerate(zip(labels, actions)): | ||
| action_name = f"ctx_{i}" |
There was a problem hiding this comment.
Does "ctx" have a special meaning that GTK understands, or can it be anything. If it can be anything, you could simply use the label instead.
| mouse_loc = AppKit.NSEvent.mouseLocation() | ||
| menu.popUpMenuPositioningItem_atLocation_inView_(None, mouse_loc, None) |
There was a problem hiding this comment.
For other backends, the position is extracted from event or event.guiEvent. So try to follow that here as well, instead of using the current mouse location. Check this for other backends as well, for instance Qt and Wx
|
|
||
| if (dx**2 + dy**2) > 5: | ||
| self._mouse_moved = True | ||
|
|
There was a problem hiding this comment.
This threshold is now not too high, but is still noticeable. If I move the cursor slightly, the plot is zoomed as well as the context menu is shown.
| def context_menu(self, event, labels=None, actions=None): | ||
| if labels is None or actions is None: | ||
| return | ||
| menu = tk.Menu(self.window, tearoff=0) | ||
| for label, action in zip(labels, actions): | ||
| menu.add_command(label=label, command=action) | ||
| menu.tk_popup(event.guiEvent.x_root, event.guiEvent.y_root) |
There was a problem hiding this comment.
Why is tk's context menu so horrible? 😂
Just check once whether you can do something about it, else lite only
There was a problem hiding this comment.
For me it looks fine only
2 participants
PR summary
PR checklist