Skip to content
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

Toolbars keep history if axes change (navtoolbar2 + toolmanager) #4857

Merged
merged 1 commit into from Jan 4, 2016

Conversation

KevKeating
Copy link
Contributor

This is a fix for #2511. With this PR, the back, forward, and home buttons in the toolbar will continue to work even if the number of axes changes. I've implemented this fix in both the navtoolbar2 toolbar as well as the MEP22 toolmanager toolbar, so this PR essentially combines #3347 and fariza#5.

The PR adds a home_views dictionary that stores the initial views of all axes. If an axes object didn't exist when a view was created, then the initial view will be used for that axes in that view. Positions will only be restored if there is a position for all current axes objects. This avoids having newer axes overlap with older ones when clicking on back or home.

I've also removed toolbar.update() as an axobserver since it's no longer necessary. (I noticed that toolbar.update() was connected in all the backends except for Qt4, which explains the traceback I observed in #2511 when using Qt4.)

@fariza
Copy link
Member

fariza commented Aug 3, 2015

@KevKeating have you seen #4795 ?

@KevKeating
Copy link
Contributor Author

I hadn't seen it, but I just took a look through it and it looks quite nice. It shouldn't be difficult to merge this PR with that one. Calls to _axes_view in this PR could simply be replaced with a call to a.get_view() instead, and

xmin, xmax, ymin, ymax = cur_lim
a.set_xlim((xmin, xmax))
a.set_ylim((ymin, ymax))

in _update_view in this PR could be replaced with a.set_view(cur_lim).

@KevKeating
Copy link
Contributor Author

I just fixed a PEP8 issue in a docstring that Travis found. There was also a Python 2.6 failure that I'll look into soon.

@jenshnielsen
Copy link
Member

I think the python 2.6 failure was just a random Travis failure. I have restarted the test

@KevKeating
Copy link
Contributor Author

Just updated this PR to use the new API from #4795.

@tacaswell tacaswell added this to the proposed next point release (2.1) milestone Oct 2, 2015
@fariza
Copy link
Member

fariza commented Oct 11, 2015

@tacaswell do you think it is pertinent just make the changes in toolmanager so no new features appear in the old ToolbarNavigation

@tacaswell
Copy link
Member

@fariza I think it is fine to only put this into toolmanager, but will defer final judgement to you on this.

# Define Home
self.push_current()
# Adding the clear method as axobserver, removes this burden from
# the backend
self.figure.add_axobserver(self.clear)
Copy link
Member

Choose a reason for hiding this comment

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

@KevKeating
If you add one axes, and don't call any of the navigation methods, your views and possitions will be out of sync.
Somebody accessing the views, positions...
Why don't leave the axobserver and instead of calling clear, calling update_home_views?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fariza
Good point. I'll add update_home_views as an axobserver. Should I remove the ToolbarNavigation changes or leave those in?

@fariza
Copy link
Member

fariza commented Oct 26, 2015

@KevKeating I really think it is better if we keep this confined to ToolManager, so we don't have to deal with new features and its bugs inside something we want to deprecate.
Please remove the changes in NavigationToolbar2

@KevKeating
Copy link
Contributor Author

Just pushed an update. update_home_views now runs as an axobserver. Since it's run when axes are added, it no longer needs to be run in push_current. I've also removed the NavigationToolbar2 changes.

# the backend
self.figure.add_axobserver(self.clear)

def clear(self, figure):
Copy link
Member

Choose a reason for hiding this comment

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

Don't remove the clear method, it could be useful for other tools.
Update it to include the update_home_views

Copy link
Member

Choose a reason for hiding this comment

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

@KevKeating I think the clear method comment that I made is the last thing needed before merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, that completely slipped my mind. I'll update the diff soon.

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 just added clear back in with

self.home_views[figure].clear()
self.update_home_views()

added. That's the only change in this new commit.

@KevKeating
Copy link
Contributor Author

I'm not sure what's going on with the test failure. It looks like assert_array_equal is raising an exception even though the tuples it's comparing are equal. (Both tuples are (0,0, 1.0), where each number is of type numpy.float64.) If I replace assert assert_array_equal(ax_lst[0][1].get_xlim(), orig_xlim) with assert ax_lst[0][1].get_xlim() == orig_xlim, then the test passes. Should I add that change to my branch?

@fariza
Copy link
Member

fariza commented Jan 4, 2016

@KevKeating Do you think it is related to your changes? I don't see how.

@KevKeating
Copy link
Contributor Author

@fariza Me neither. I'm guessing that it's unrelated.

fariza added a commit that referenced this pull request Jan 4, 2016
Toolbars keep history if axes change (navtoolbar2 + toolmanager)
@fariza fariza merged commit c849398 into matplotlib:master Jan 4, 2016
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.

None yet

4 participants