Skip to content

Wx Toolbar for ToolManager #10851

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

Merged
merged 5 commits into from
Jul 9, 2018

Conversation

DietmarSchwertberger
Copy link
Contributor

PR Summary

This adds a first implementation of the ToolbarWx for use with ToolManager, including StatusbarWx, and ConfigureSubplotsWx.
The other tools were there already.
The ToolManager example is updated.

Additionally, an icon will be set also for FigureFrameWx.

I will also prepare an additional PR to replace the toolbar with a standard wx toolbar at the top of the frame. This will fix issue 2716 and make the FigureFrameWx initialization code more straightforward.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

self, name, group, position, image_file, description, toggle):

before, group = self._add_to_group(group, name, position)
idx = self.GetToolPos(before.Id)
Copy link
Member

Choose a reason for hiding this comment

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

where is this method defined? Pep8 lowercase

Copy link
Member

Choose a reason for hiding this comment

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

I guess it comes from wx.Toolbar, forget the previous comment

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, it went out as review... I didn't know how to cancel the "start a review"

@fariza
Copy link
Member

fariza commented Mar 21, 2018

I have to install my wxpython correctly before I can continue the review. Soon...

@DietmarSchwertberger
Copy link
Contributor Author

@fariza : That should not be too hard. If you're on Linux, then you should look here, as otherwise PIP will build from sources: https://wxpython.org/pages/downloads/

@@ -1337,6 +1360,26 @@ def _load_bitmap(filename):
return bmp


_FRAME_ICON = None
Copy link
Member

Choose a reason for hiding this comment

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

I'm not that fond of this Gloabal icon.
It seems to be there just to save loading again the same image.
Unless we are short on memory or it does impose a penalty on performance, I would say, just load it again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually a set of bitmaps with different sizes. As actually there are only two, I'm fine with reloading. I've just pushed the modification.

Copy link
Member

@fariza fariza left a comment

Choose a reason for hiding this comment

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

I did several tests and everything seems to work as expected

@fariza
Copy link
Member

fariza commented Apr 9, 2018

@anntzer can you help reviewing this?

@anntzer
Copy link
Contributor

anntzer commented Apr 9, 2018

Would be much happier if (e.g.) https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/tests/test_backends_interactive.py was modified to add testing for the tool manager (right now there's no CI at all on any of this). Note that this could be done independently of (and before) this PR.

@anntzer anntzer mentioned this pull request Apr 9, 2018
@DietmarSchwertberger
Copy link
Contributor Author

For me, that's currently out of scope. I'm not familar enough with this yet.

@anntzer
Copy link
Contributor

anntzer commented Apr 9, 2018

(I'm fine if others want to review this and get it in; I would just be more comfortable if this was tested by CI. Otherwise, I would want to find time to test all the features myself, which I don't have the bandwidth for right now.)

@anntzer
Copy link
Contributor

anntzer commented Apr 19, 2018

Needs a rebase.

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

needs rebase, but otherwise looks good.

@anntzer
Copy link
Contributor

anntzer commented Apr 19, 2018

Let's just try to keep things moving and push back the discussion to when backend_tools becomes the default, if that ever happens...

@fariza
Copy link
Member

fariza commented Apr 20, 2018

There is a problem. I think it is in the doc building steps

autodoc: failed to import module 'matplotlib.backends.backend_wxagg'; the following exception was raised:
Traceback (most recent call last):
  File "/home/circleci/.local/lib/python3.6/site-packages/sphinx/ext/autodoc/importer.py", line 140, in import_module
    __import__(modname)
  File "/home/circleci/project/lib/matplotlib/backends/backend_wxagg.py", line 7, in <module>
    from .backend_wx import (
  File "/home/circleci/project/lib/matplotlib/backends/backend_wx.py", line 1795, in <module>
    class StatusbarWx(StatusbarBase, wx.StatusBar):
TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

@DietmarSchwertberger
Copy link
Contributor Author

Unfortunately, I don't know how to interpret this message or how to fix it.
There are no metaclasses involved.
The inheritance structure for StatusbarWx is the same as e.g. for ToolbarWx which does not trigger the message:

class ToolbarWx(ToolContainerBase, wx.ToolBar):
    def __init__(self, toolmanager, parent, style=wx.TB_HORIZONTAL):
        ToolContainerBase.__init__(self, toolmanager)
        wx.ToolBar.__init__(self, parent, -1, style=style)
   ...

class StatusbarWx(StatusbarBase, wx.StatusBar):
    def __init__(self, parent, *args, **kwargs):
        StatusbarBase.__init__(self, *args, **kwargs)
        wx.StatusBar.__init__(self, parent, -1)

@anntzer
Copy link
Contributor

anntzer commented Apr 21, 2018

That's due to the slightly atrocious mocking we use to make docs build even though wx is not installed (at https://github.com/matplotlib/matplotlib/blob/master/doc/sphinxext/mock_gui_toolkits.py#L111). The solution is to remove the API docs for wx (similarly to https://github.com/matplotlib/matplotlib/pull/9993/files#diff-dd295241396a7490c85d37c53ae65dc2) and (not structly necessary, but may as well) remove the mocking mentioned above.
As argued elsewhere, the docs for the GUI integrations should be replaced by a single hand-written docs documenting the common API (and critical differences, when applicable) of all toolkits anyways...

@DietmarSchwertberger
Copy link
Contributor Author

This change makes the test pass, but then the documentation would be gone, right?
Do you think that adding a StatusBar to class MyWX(MagicMock) would work?
That seems to be the difference between ToolBar and StatusBar.

@DietmarSchwertberger
Copy link
Contributor Author

OK, I've added the StatusBar. The two ci/circleci still fail, but with something where I would guess it's not related to the PR.

@anntzer
Copy link
Contributor

anntzer commented Apr 24, 2018

The doc build failure is #11107, please fetch, rebase, and push.
By the way, feel free to remove the "needs rebase" label after a rebase, makes it easier to keep track of PR status.

@DietmarSchwertberger
Copy link
Contributor Author

I'm still not familiar with git and still looking for a better GUI tool. I probably should have used "fetch" instead of "pull". The commit history looks like a mess, but the code diff is correct.

This did re-trigger the ci/circleci, but they still fail....

@QuLogic
Copy link
Member

QuLogic commented Apr 26, 2018

You have extra commits because you did some weirdness when rebasing. There are actually two copies of your commits now, one ending at e68cfac that's the original, and one ending at a601003 that was rebased yesterday.

When you rebase and try to push, git tells you to fetch & merge (or pull) first. While this is the safe option, it's also wrong in this case because it pulls the old copy of your commits. You want to replace the old commits with the rebased ones, so you need to do git push --force origin wxtoolmanager.

This is not too difficult to fix (these instructions assume you have no other commits or changes):

git checkout wxmanager
git reset --hard a60100352f12916541a4a7521d7a27a1ecf50696
git push --force origin wxmanager

However, you have a pair of commits that do something and then immediately revert it; I would also take this opportunity to remove those. After the reset, do git rebase --interactive upstream/master, and your editor will popup with a list of commits. Remove the avoid 'TypeError: metaclass ... line and its revert, then exit the editor. Then git will replay your commits and leave out those two. You can follow up with the force-push after that.

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Please remove duplicate commits.

@DietmarSchwertberger
Copy link
Contributor Author

@QuLogic thanks a lot. That looks much better now. I think the "--force" was the problem. I could not imagine that such a standard task would require such a flag. A nice GUI with preview etc. would be great...

@timhoffm
Copy link
Member

Concerning --force: Well, you rewrote the history. This is potentially harmful if multiple people work on that branch. You should only ever rewrite the history if you're the only using the branch. --force is your explicitly consent that you know what you are doing.

@DietmarSchwertberger
Copy link
Contributor Author

@QuLogic : as the changes are done, would you mind updating your review result? Or maybe even merge the PR? Once this and PR #11081 are merged, I would like to start work on Wayland support to fix issue #10417

@QuLogic QuLogic dismissed their stale review May 23, 2018 01:42

History re-written.

@tacaswell tacaswell merged commit 99cb3dd into matplotlib:master Jul 9, 2018
@tacaswell
Copy link
Member

Thanks @DietmarSchwertberger , sorry these sat so long.

@DietmarSchwertberger DietmarSchwertberger deleted the wxtoolmanager branch July 13, 2018 18:31
@DietmarSchwertberger
Copy link
Contributor Author

@tacaswell : no problem. Thanks for merging. Maybe the guidelines should be updated to encourage self merging when two reviewers have approved.
Anyway, I've been quite bit busy with other projects recently. After 3.0 I will create a PR for issue #2716 and then a PR for an improved wx backend including Windows Metafile support. This should be the basis for the discussion whether to keep or finally remove the wx backend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants