-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Wx Toolbar for ToolManager #10851
Conversation
self, name, group, position, image_file, description, toggle): | ||
|
||
before, group = self._add_to_group(group, name, position) | ||
idx = self.GetToolPos(before.Id) |
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.
where is this method defined? Pep8 lowercase
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.
I guess it comes from wx.Toolbar, forget the previous comment
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.
Sorry, it went out as review... I didn't know how to cancel the "start a review"
I have to install my wxpython correctly before I can continue the review. Soon... |
@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 |
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.
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
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'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.
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.
I did several tests and everything seems to work as expected
@anntzer can you help reviewing this? |
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. |
For me, that's currently out of scope. I'm not familar enough with this yet. |
(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.) |
Needs a rebase. |
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.
needs rebase, but otherwise looks good.
Let's just try to keep things moving and push back the discussion to when backend_tools becomes the default, if that ever happens... |
There is a problem. I think it is in the doc building steps
|
Unfortunately, I don't know how to interpret this message or how to fix it.
|
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. |
This change makes the test pass, but then the documentation would be gone, right? |
OK, I've added the |
The doc build failure is #11107, please fetch, rebase, and push. |
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.... |
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 This is not too difficult to fix (these instructions assume you have no other commits or changes):
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 |
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.
Please remove duplicate commits.
d043343
to
71043a6
Compare
@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... |
Concerning |
Thanks @DietmarSchwertberger , sorry these sat so long. |
@tacaswell : no problem. Thanks for merging. Maybe the guidelines should be updated to encourage self merging when two reviewers have approved. |
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