Skip to content

MEP22 Navigation toolbar coexistence TODELETE #2759

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.

Sign up for GitHub

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

Closed
wants to merge 38 commits into from
Closed

MEP22 Navigation toolbar coexistence TODELETE #2759

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
dda0cdc
navigation and toolbar coexistence
fariza Jan 23, 2014
10f5dc7
mod keypress in figuremanager
fariza Jan 23, 2014
08a6020
extra files
fariza Jan 23, 2014
c6c0ad3
helper methods in toolbar and navigation
fariza Jan 24, 2014
1fc29fa
Adding doc to base methods
fariza Jan 24, 2014
979875e
property for active_toggle
fariza Jan 26, 2014
c5c4f0f
simulate click
fariza Jan 27, 2014
97dfda7
pep8 backend_tools
fariza Jan 27, 2014
6b647ad
activate renamed to trigger
fariza Jan 28, 2014
99667aa
toggle tools using enable/disable from its trigger method
fariza Jan 29, 2014
6c19579
simplifying _handle_toggle
fariza Jan 29, 2014
0495aac
reducing number of locks
fariza Jan 29, 2014
fb46fc1
pep8 correction
fariza Jan 29, 2014
d49c431
changing toggle and persistent attributes for issubclass
fariza Feb 4, 2014
5ba6210
bug in combined key press
fariza Feb 4, 2014
7ca8626
untoggle zoom and pan from keypress while toggled
fariza Feb 4, 2014
dcc0f16
classmethods for default tools modification
fariza Feb 6, 2014
bc703e0
six fixes
fariza Apr 24, 2014
68dc711
adding zaxis and some pep8
fariza May 1, 2014
bb9f1c7
removing legacy method dynamic update
fariza May 6, 2014
2c2e649
tk backend
fariza May 6, 2014
a99367f
pep8
fariza May 6, 2014
3d1be34
example working with Tk
fariza May 6, 2014
afdd34c
cleanup
fariza May 7, 2014
5b49c7a
duplicate code in keymap tool initialization
fariza Jul 24, 2014
773db88
grammar corrections
fariza Jul 24, 2014
2ca6926
moving views and positions to tools
fariza Jul 24, 2014
661417d
The views positions mixin automatically adds the clear as axobserver
fariza Jul 25, 2014
90ab64f
bug when navigation was not defined
fariza Jul 25, 2014
55dd149
Small refactor so that we first initiate the Navigation (ToolManager)…
OceanWolf Jul 28, 2014
15ac091
Update for Sphinx documentation
OceanWolf Jul 28, 2014
8cd241c
Moved default_tool initilisation to FigureManagerBase and cleaned.
OceanWolf Jul 29, 2014
39f5b74
Fix navigation
OceanWolf Jul 29, 2014
b20dade
Temporary fix to backends
OceanWolf Jul 29, 2014
ff94301
Merge pull request #1 from OceanWolf/navigation-toolbar-coexistence-e…
fariza Jul 29, 2014
2cb4501
removing persistent tools
fariza Sep 3, 2014
9d3c977
removing unregister
fariza Sep 4, 2014
6e0b7e6
change cursor inmediately after toggle
fariza Sep 5, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
removing persistent tools
  • Loading branch information
@fariza
fariza committed Sep 3, 2014
commit 2cb4501ee7eabf40b94bc604c16e8c1f8351bbcf
2 changes: 1 addition & 1 deletion examples/user_interfaces/navigation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,6 @@ def trigger(self, event):
fig.canvas.manager.navigation.add_tool('copy', CopyToolGTK3)

# Just for fun, lets remove the back button
fig.canvas.manager.navigation.remove_tool('Back')
# fig.canvas.manager.navigation.remove_tool('Back')

plt.show()
59 changes: 14 additions & 45 deletions lib/matplotlib/backend_bases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3212,7 +3212,6 @@ def __init__(self, manager):

self._tools = {}
self._keys = {}
self._instances = {}
self._toggled = None

# to process keypress event
Expand All @@ -3231,15 +3230,6 @@ def active_toggle(self):

return self._toggled

@property
def instances(self):
"""Active tools instances

**dictionary** : Contains the active instances that are registered
"""

return self._instances

def get_tool_keymap(self, name):
"""Get the keymap associated with a tool

Expand Down Expand Up @@ -3281,28 +3271,19 @@ def set_tool_keymap(self, name, *keys):
self._keys[k] = name

def unregister(self, name):
"""Unregister the tool from the active instances
"""Unregister the tool from Navigation

Parameters
----------
name : string
Name of the tool to unregister

Notes
-----
This method is used by `PersistentTools` to remove the reference kept
by `Navigation`.

It is usually called by the `unregister` method

If called, next time the `Tool` is used it will be reinstantiated
instead of using the existing instance.
"""

if self._toggled == name:
self._handle_toggle(name, from_toolbar=False)
if name in self._instances:
del self._instances[name]
if name in self._tools:
self._tools[name].destroy()
del self._tools[name]

def remove_tool(self, name):
"""Remove tool from the `Navigation`
Expand All @@ -3314,7 +3295,7 @@ def remove_tool(self, name):
"""

self.unregister(name)
del self._tools[name]

keys = [k for k, v in six.iteritems(self._keys) if v == name]
for k in keys:
del self._keys[k]
Expand Down Expand Up @@ -3361,7 +3342,7 @@ def add_tool(self, name, tool, position=None):
'not added')
return

self._tools[name] = tool_cls
self._tools[name] = tool_cls(self.canvas.figure, name)
if tool_cls.keymap is not None:
self.set_tool_keymap(name, tool_cls.keymap)

Expand Down Expand Up @@ -3394,27 +3375,23 @@ def _get_cls_to_instantiate(self, callback_class):

return callback_class

def trigger_tool(self, name):
def trigger_tool(self, name, event=None):
"""Trigger on a tool

Method to programatically "click" on Tools
"""

self._trigger_tool(name, None, False)
self._trigger_tool(name, event, False)

def _trigger_tool(self, name, event, from_toolbar):
if name not in self._tools:
raise AttributeError('%s not in Tools' % name)

tool = self._tools[name]
if issubclass(tool, tools.ToolToggleBase):
if isinstance(tool, tools.ToolToggleBase):
self._handle_toggle(name, event=event, from_toolbar=from_toolbar)
elif issubclass(tool, tools.ToolPersistentBase):
instance = self._get_instance(name)
instance.trigger(event)
else:
# Non persistent tools, are instantiated and forgotten
tool(self.canvas.figure, event)
tool.trigger(event)

def _key_press(self, event):
if event.key is None or self.keypresslock.locked():
Expand All @@ -3425,14 +3402,6 @@ def _key_press(self, event):
return
self._trigger_tool(name, event, False)

def _get_instance(self, name):
if name not in self._instances:
instance = self._tools[name](self.canvas.figure, name)
# register instance
self._instances[name] = instance

return self._instances[name]

def _toolbar_callback(self, name):
"""Callback for the `Toolbar`

Expand All @@ -3453,7 +3422,7 @@ def _handle_toggle(self, name, event=None, from_toolbar=False):
if not from_toolbar and self.toolbar:
self.toolbar._toggle(name, False)
Copy link
Member

Choose a reason for hiding this comment

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

I find this from_toolbar argument messy, it shouldn't matter where the request to toggle the tool between an on/off state came from. I can see why you did it but I think, we don't need it, basically rewrite the above if block as:

if self.toolbar:
  self.toolbar._set_state(name, self._toggled != name, False)

and create the extra method in Toolbar{Base,GTK3,TK}

Copy link
Member Author

Choose a reason for hiding this comment

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

That implies more work for the people creating the toolbars.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but not too much extra work, only a 5 lines of extra code, per backend I would imagine; and makes the method calls clearer, especially in the future if/when other methods for selecting tools come about.

If the extra work remains the only concern, then the _toggle method could get taken out of the specific backends entirely, replacing the _toggle method with a _set_state method, with ToolbarBase still having a toggle method, but it can do the logic of which state to set instead of the specific toolbar. Taking ToolbarGTK3 as an example this would use even less code.

Taking these ideas one step further, what do you think about replacing my suggested codeblock above with something more like:

event = ToolStateChangedEvent(tool)
self.callbacks.process('tool_state_changed_event', event)

where the argument passed to ToolStateChangedEvent, either the name and the state, or the tool itself (which should encapsulate those two items).

Through something like this we generalise the toggling mechanism, and allow for an arbitrary number of observers, say for example someone in the future wants to add a right click menu with tools inside.

Copy link
Member Author

Choose a reason for hiding this comment

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

@OceanWolf can you try to make a PR against my branch with this change?
Please include only this change so I can review it easily.

Quoting #2624 (comment)

@fariza If you have a local copy of my branch, make your changes on the tip and then push your branch to your github repo (just like making changes against master).

The PR interface in github has issues with large networks. The advice they gave us was to edit the URL by hand to point the PR against the correct repo/branch.

Copy link
Member

Choose a reason for hiding this comment

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

Sure thing :). Looking forward to it, my first github pull request :D.

Copy link
Member Author

Choose a reason for hiding this comment

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

the only difference is that you create a branch based on my branch, and that the PR is against my branch (on my repo) not master (on matplotlib repo)

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually for setting the fork, is
http://matplotlib.org/devel/gitwash/forking_hell.html#forking
http://matplotlib.org/devel/gitwash/set_up_fork.html#set-up-fork

Then you go to the installation, and then to the workflow.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I have the navigation example working! :D. Thank you for your help!

To clarify, by:

Please include only this change so I can review it easily.

What did you consider as this change? As I have started coding, I have realised that:

  1. The event framework should get moved out of FigureManagerBase and into a mixin, otherwise I would duplicate code.
  2. The most elegant solution IMO lies in making the toolbar aware of its tools as discussed previously, i.e. in ToolbarBase the following rough code:
def addTool(self, name, tool)
  tool.mpl_connect('tool_state_changed_event', self.set_state)
  self._add_toolitem(self, name, tool.description, ...):

Otherwise:

  • ToolbarBase needs to know of all Navigation, and thus has access to all tools, and...
  • The toolbar gets notified of tool status changes even if the tool does not exist within the toolbar, which I see happens at the moment and gives an error in the message area of the toolbar, which seems like an unneeded notification, and repetitive code for the toolbar which the specific backend people will have to write.

Copy link
Member Author

Choose a reason for hiding this comment

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

@OceanWolf
The event framework has to remain in FigureManagerBase. If you want, later it can be addressed with other MEP. That is too big of a change to be included in this MEP.
I know it is tempting to solve all the problems in one shot ;).

Keep in mind the MAIN GOAL is to make it easy to create Tools, and Toolbars, and to reconfigure them. Navigation is just a side effect needed to get there.
I think we are there, we attain the main goal, currently we are just sanding the corners to see if the core developers like what they see.

Copy link
Member

Choose a reason for hiding this comment

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

@fariza Yes, I see what you mean. Very tempting indeed, for me because of the desire to keep backward comparability, knowing that when other problems get solved, it may well in turn enable a better/clearer API for tools.

I have made quite a bit of progress, so I shall keep going a bit further to see the end result, if I may. Not too much further to go :).


instance = self._get_instance(name)
tool = self._tools[name]
if self._toggled is None:
# first trigger of tool
self._toggled = name
Expand All @@ -3465,10 +3434,10 @@ def _handle_toggle(self, name, event=None, from_toolbar=False):
if self.toolbar:
# untoggle the previous toggled tool
self.toolbar._toggle(self._toggled, False)
self._get_instance(self._toggled).trigger(event)
self._tools[self._toggled].trigger(event)
self._toggled = name

instance.trigger(event)
tool.trigger(event)

for a in self.canvas.figure.get_axes():
a.set_navigate_mode(self._toggled)
Expand All @@ -3492,7 +3461,7 @@ def _mouse_move(self, event):
self._last_cursor = self._default_cursor
else:
if self._toggled:
cursor = self._instances[self._toggled].cursor
cursor = self._tools[self._toggled].cursor
if cursor and self._last_cursor != cursor:
self.set_cursor(cursor)
self._last_cursor = cursor
Expand Down
Loading