-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
MEP22: Navigation by events #3652
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
Changes from 1 commit
8cceed4
3118a5a
b4d5fcf
1e8af47
622cb95
d1a9de4
3f89d52
4f3c10b
6065daa
f6a2f19
05db3b6
c08fe56
b207a72
9266447
a53419a
704c717
5056729
e6a4e1e
8942c47
022de6f
2c9a195
cafe668
224f745
94c711e
67257e7
ffa65d6
6739ee0
d18206f
34a52c8
c2da483
44a9b0e
a2ed47f
0665890
411e6e2
d484ebd
75bf97b
6cc040b
0ff5997
af6734f
78513d2
377ff54
7dbbf58
dd66b57
67a414f
e415d8d
1213086
ba61dec
9f2ee2b
9da2b13
110253f
e2804ea
9a64b7e
64f947f
e8cd5d5
4bbcf4e
73a2661
1b83628
e4edd23
d4ac2fb
a7640ef
48a6971
8dafe09
a0695d0
328b169
aac4744
f09b9ef
def3a52
9ee7e25
5eae4e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,7 @@ | |
user interaction (key press, toolbar clicks, ..) and the actions in | ||
response to the user inputs. | ||
|
||
:class:`ToolbarBase` | ||
:class:`ToolContainerBase` | ||
The base class for the Toolbar class of each interactive backend. | ||
|
||
""" | ||
|
@@ -3399,18 +3399,16 @@ def remove_tool(self, name): | |
del self._tools[name] | ||
|
||
def add_tools(self, tools): | ||
""" Add multiple tools to `Navigation` | ||
""" Add multiple tools to `NavigationBase` | ||
|
||
Parameters | ||
---------- | ||
tools : List | ||
List in the form | ||
[(Tool1, name1), (Tool2, name2) ...] | ||
where Tool1, name1 represent the tool, and the respective name | ||
of the tool which gets used as an id. | ||
tools : {str: class_like} | ||
The tools to add in a {name: tool} dict, see `add_tool` for more | ||
info. | ||
""" | ||
|
||
for tool, name in tools: | ||
for name, tool in six.iteritems(tools): | ||
self.add_tool(name, tool) | ||
|
||
def add_tool(self, name, tool, *args, **kwargs): | ||
|
@@ -3424,14 +3422,18 @@ def add_tool(self, name, tool, *args, **kwargs): | |
|
||
Parameters | ||
---------- | ||
name : string | ||
name : str | ||
Name of the tool, treated as the ID, has to be unique | ||
tool : string or `matplotlib.backend_tools.ToolBase` derived class | ||
Reference to find the class of the Tool to be added | ||
tool : class_like, i.e. str or type | ||
Reference to find the class of the Tool to added. | ||
|
||
Notes | ||
----- | ||
args and kwargs get passed directly to the tools constructor. | ||
|
||
See Also | ||
-------- | ||
matplotlib.backend_tools.ToolBase : The base class for tools. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't we need backticks around class names to make them links? Also, it used to be that we needed to prepend with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I can see, in the |
||
""" | ||
|
||
tool_cls = self._get_cls_to_instantiate(tool) | ||
|
@@ -3442,7 +3444,7 @@ def add_tool(self, name, tool, *args, **kwargs): | |
if name in self._tools: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be done first in this function? Do we want to check to make sure that they are the same type? Again, why warn rather than raise? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used warn because i don't think that adding twice the same tool represents a major fault and your program will keep working as expected (you already have the tool) |
||
warnings.warn('A tool_cls with the same name already exist, ' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a user won't know what Also, "exist" --> "exists". |
||
'not added') | ||
return | ||
return self._tools[name] | ||
|
||
self._tools[name] = tool_cls(self, name, *args, **kwargs) | ||
|
||
|
@@ -3573,30 +3575,32 @@ def tools(self): | |
|
||
return self._tools | ||
|
||
def get_tool(self, name): | ||
def get_tool(self, name, warn=True): | ||
"""Return the tool object, also accepts the actual tool for convenience | ||
|
||
Parameters | ||
----------- | ||
name : String, ToolBase | ||
name : str, ToolBase | ||
Name of the tool, or the tool itself | ||
warn : bool | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
If this method should give warnings. | ||
""" | ||
if isinstance(name, tools.ToolBase) and tool.name in self._tools: | ||
if isinstance(name, tools.ToolBase) and name.name in self._tools: | ||
return name | ||
if name not in self._tools: | ||
warnings.warn("%s is not a tool controlled by Navigation" % name) | ||
if warn: | ||
warnings.warn("Navigation does not control tool %s" % name) | ||
return None | ||
return self._tools[name] | ||
|
||
|
||
class ToolbarBase(object): | ||
"""Base class for `Toolbar` implementation | ||
class ToolContainerBase(object): | ||
"""Base class for all tool containers, e.g. toolbars. | ||
|
||
Attributes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't lined up with the underscores below There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
---------- | ||
manager : `FigureManager` object that integrates this `Toolbar` | ||
navigation : `NavigationBase` object that hold the tools that | ||
this `Toolbar` wants to communicate with | ||
navigation : `NavigationBase` object that holds the tools that | ||
this `ToolContainer` wants to communicate with. | ||
""" | ||
|
||
def __init__(self, navigation): | ||
1D70
|
@@ -3618,51 +3622,38 @@ def _tool_toggled_cbk(self, event): | |
self.toggle_toolitem(event.tool.name, event.tool.toggled) | ||
|
||
def add_tools(self, tools): | ||
""" Add multiple tools to `Navigation` | ||
""" Add multiple tools to the container. | ||
|
||
Parameters | ||
---------- | ||
tools : List | ||
tools : list | ||
List in the EE58 form | ||
[[group1, [name1, name2 ...]][group2...]] | ||
where group1 is the name of the group where the | ||
Tool1, Tool2... are going to be added, and name1, name2... are the | ||
names of the tools | ||
[[group1, [tool1, tool2 ...]], [group2, [...]]] | ||
Where the tools given by tool1, and tool2 will display in group1. | ||
See `add_tool` for details. | ||
""" | ||
|
||
for group, grouptools in tools: | ||
for position, tool in enumerate(grouptools): | ||
self.add_tool(tool, group, position) | ||
|
||
def add_tool(self, tool, group, position=-1, name=None, **kwargs): | ||
"""Adds a tool to the toolbar | ||
def add_tool(self, tool, group, position=-1): | ||
"""Adds a tool to this container | ||
|
||
Parameters | ||
---------- | ||
tool : string, tool | ||
The name or the type of tool to add. | ||
group : string | ||
tool : tool_like | ||
The tool to add, see `NavigationBase.get_tool`. | ||
group : str | ||
The name of the group to add this tool to. | ||
position : int | ||
the relative position within the group to place this tool. | ||
name : string (optional) | ||
If given, and the above fails, we use this to create a new tool of | ||
type given by tool, and use this as the name of the tool. | ||
""" | ||
t = self.navigation.get_tool(tool) | ||
if t is None: | ||
if isinstance(tool, type): | ||
tool = tool.__class__ | ||
if name is not None: | ||
t = self.navigation.add_tool(name, tool, **kwargs) | ||
if t is None: | ||
warning.warn('Cannot add tool %s'%tool) | ||
return | ||
tool = t | ||
position : int (optional) | ||
The position within the group to place this tool. Defaults to end. | ||
""" | ||
tool = self.navigation.get_tool(tool) | ||
image = self._get_image_filename(tool.image) | ||
toggle = getattr(tool, 'toggled', None) is not None | ||
self.add_toolitem(tool.name, group, position, image, | ||
tool.description, toggle) | ||
self.add_toolitem(tool.name, group, position, | ||
image, tool.description, toggle) | ||
if toggle: | ||
self.navigation.nav_connect('tool_trigger_%s' % tool.name, | ||
self._tool_toggled_cbk) | ||
|
@@ -3688,13 +3679,13 @@ def trigger_tool(self, name): | |
Parameters | ||
---------- | ||
name : String | ||
Name(id) of the tool triggered from within the toolbar | ||
Name(id) of the tool triggered from within the container | ||
|
||
""" | ||
self.navigation.tool_trigger_event(name, sender=self) | ||
|
||
def add_toolitem(self, name, group, position, image, description, toggle): | ||
"""Add a toolitem to the toolbar | ||
"""Add a toolitem to the container | ||
|
||
This method must get implemented per backend | ||
|
||
|
@@ -3734,18 +3725,20 @@ def set_message(self, s): | |
|
||
pass | ||
|
||
def toggle_toolitem(self, name): | ||
def toggle_toolitem(self, name, toggled): | ||
"""Toggle the toolitem without firing event | ||
|
||
Parameters | ||
---------- | ||
name : String | ||
Id of the tool to toggle | ||
toggled : bool | ||
Whether to set this tool as toggled or not. | ||
""" | ||
raise NotImplementedError | ||
|
||
def remove_toolitem(self, name): | ||
"""Remove a toolitem from the `Toolbar` | ||
"""Remove a toolitem from the `ToolContainer` | ||
|
||
This method must get implemented per backend | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -903,23 +903,25 @@ def _mouse_move(self, event): | |
self.navigation.canvas.draw_idle() | ||
|
||
|
||
tools = [(ToolHome, 'home'), (ToolBack, 'back'), (ToolForward, 'forward'), | ||
(ToolZoom, 'zoom'), (ToolPan, 'pan'), | ||
('ToolConfigureSubplots', 'subplots'), | ||
('ToolSaveFigure', 'save'), | ||
(ToolGrid, 'grid'), | ||
(ToolFullScreen, 'fullscreen'), | ||
(ToolQuit, 'quit'), | ||
(ToolEnableAllNavigation, 'allnav'), | ||
(ToolEnableNavigation, 'nav'), | ||
(ToolXScale, 'xscale'), | ||
(ToolYScale, 'yscale'), | ||
(ToolCursorPosition, 'position'), | ||
(ToolViewsPositions, 'viewpos'), | ||
('ToolSetCursor', 'cursor'), | ||
('ToolRubberband', 'rubberband')] | ||
tools = {'home': ToolHome, 'back': ToolBack, 'forward': ToolForward, | ||
'zoom': ToolZoom, 'pan': ToolPan, | ||
'subplots': 'ToolConfigureSubplots', | ||
'save': 'ToolSaveFigure', | ||
'grid': ToolGrid, | ||
'fullscreen': ToolFullScreen, | ||
'quit': ToolQuit, | ||
'allnav': ToolEnableAllNavigation, | ||
'nav': ToolEnableNavigation, | ||
'xscale': ToolXScale, | ||
'yscale': ToolYScale, | ||
'position': ToolCursorPosition, | ||
'viewpos': ToolViewsPositions, | ||
'cursor': 'ToolSetCursor', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are some of these values strings and some of them classes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The string values are going to be implemented by the backend and |
||
'rubberband': 'ToolRubberband'} | ||
"""Default tools""" | ||
|
||
toolbar_tools = [['navigation', ['home', 'back', 'forward']], | ||
['zoompan', ['zoom', 'pan']], | ||
['layout', ['subplots']], | ||
['io', ['save']]] | ||
"""Default tools""" | ||
"""Default tools in the toolbar""" |
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.
How strongly do you feel about keeping both the singular and plural versions of this? The similarity (leading to many typos) and the difference in signature/what they can do is going to be confusing. If this function stays I think it needs to be able to pass args/kwargs through to the constructors.
Is there any terrifying input parsing we could use to mush this into one function? I am not seeing a way quickly...
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.
@tacaswell, me and @fariza had a discussion on args/kwargs etcetrra over in fariza#11 where I introduced them.
I would have thought the difference in signature would solve the typos problem, I mean if you make a typo, you will get a python error telling you you passed the wrong number of arguments to the function.
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.
We can think of renaming the methods (suggestions any one?), but playing magic with arguments is 👎
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.
great, sold on 👎 for args magic (we have it else where where I do not like it, but supposedly users like that sort of thing)
What is the use case for bulk adding tools? Writing this loop in user code does not strike me as super onerous.
My worry with the typo is getting an exception about args being wrong (which leads you to check what you are passing in) rather than an
AttributeError
(which leads you to check your spelling).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.
It is more for backend implementation. It was done automatically at
__init__
but @OceanWolf talked me out of thatThere 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.
Ooh, brain wave, how about:
Or get rid of it altogether, I think it has changed a lot (becoming a lot simpler), especially since we split the adding of tools to the toolbar out and into
ToolContainer.addTool
as @fariza mentions.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.
@OceanWolf where do you suggest to add that loop?
Don't you like the helper functions in
backend_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.
👍 On putting this helper function as a module-level function. Breaks the OO abstraction a bit, but it keeps the base objects simpler.
@OceanWolf That would unpack as
self.add_tool(tool_name, value_in_tools_dict)
which does not get you the next level of unpacking you need to pass toadd_tool
.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.
Yes, I like that too, brain just sluggish and getting distracted by quite a few conversations. I don't mind, just so long as no core-logic changes.
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.
done