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

MEP22 implementation for QT backend #9934

Merged
merged 3 commits into from Feb 5, 2018

Conversation

Projects
None yet
6 participants
@fariza
Copy link
Member

commented Dec 5, 2017

PR Summary

Continuing with MEP22 development, here is the implementation for QT4/5

New Gui components

  • class ToolbarQt compatible with MEP22 ToolContainerBase
  • class StatusbarQt compatible with MEP22 StatusbarBase

New tools

  • ConfigureSubplotsQt
  • SaveFigureQt
  • SetCursorQt
  • RubberbandQt

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

To test it

Check out this branch and run

import matplotlib

matplotlib.use('QT5AGG')
matplotlib.rcParams['toolbar'] = 'toolmanager'
import matplotlib.pyplot as plt

plt.plot([1,2,3])
plt.show()
Federico Ariza
MEP22 implementation for QT backend
New Toolbar and QT specific tools
@fariza

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2017

@tacaswell @anntzer @OceanWolf
Please check it out and let me know what you think
I know I said I wanted to wait for MEP27 but I had some requests for this, so why to wait

@tacaswell tacaswell added this to the v2.2 milestone Dec 5, 2017

@anntzer

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2017

In general looks fine to me, although my comments in #8712 still stand :)

@fariza

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2017

@anntzer how would you want to proceed?
Do you want me to address your comments in #8712 first?

@anntzer

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2017

I guess it's fine for this to go in (when properly reviewed) as any change per #8712 will require changing all backends anyways...

Federico Ariza
@OceanWolf

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2017

Well that was the point of waiting @anntzer, to get #8712 and MEP27 finished first because at the moment we don't have any other backends to change. Only Gtk3 exists as a test backend with the aim of getting Gtk3 finished before moving on to other backends.

Sorry @fariza, I haven't been much use of late. real-life... I do make headway though in reducing the length of my todo list, so hopefully back before not too long :).

@fariza

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2018

@anntzer @OceanWolf Can we move ahead with this and get the important people to play with MEP22?

Most of the matplotlib maintainers use the QT backend, if we get this implementation in, we can get them to try it. Then the other pending points #9941, #9966 hopefully will get more attention.
Even we can hope MEP27 will finally get the review that it deserves

It is my fault for pushing MEP22 with GTK3 instead of QT, but too late to complain

@anntzer

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2018

quick skim looks good, will need more time to do proper review

@tacaswell

This comment has been minimized.

Copy link
Member

commented Jan 5, 2018

@fariza Thanks for pushing on this and no worries about starting with gtk3 if that is what you use day-to-day!

self.toolbar.message.connect(self.statusbar_label.setText)
if not self.toolmanager:
# add text label to status bar
statusbar_label = QtWidgets.QLabel()

This comment has been minimized.

Copy link
@jklymak

jklymak Jan 24, 2018

Contributor

This code used to happen regardless of self.toolbar. Is it OK that it only happens if its none?

This comment has been minimized.

Copy link
@fariza

fariza Jan 24, 2018

Author Member

toolmanager and the old navigation toolbar are different. So that widget is initialized only if there is no toolmanager

This comment has been minimized.

Copy link
@jklymak

jklymak Jan 24, 2018

Contributor

Yes, but can self.toolbar be None? Because even if it was, the old lines 492-494 were called. Now they aren't.

This comment has been minimized.

Copy link
@fariza

fariza Jan 24, 2018

Author Member

I really don't know if that was a mistake before.
The reasoning behind is: If you don't want a toolbar, you don't want a statusbar, we don't have separate controls but maybe we should

This comment has been minimized.

Copy link
@jklymak

jklymak Jan 24, 2018

Contributor

Isn't the status bar the thing at the top that says "Figure 1"? I think you want that regardless of whether there is a toolbar... Or is it something else?

del self._toolitems[name]


class StatusbarQt(StatusbarBase, QtWidgets.QLabel):

This comment has been minimized.

Copy link
@jklymak

jklymak Jan 24, 2018

Contributor

Sorry, maybe I need to read the MEP, but all these buttons existed in Qt before. How did they get set before and do these new buttons supercede the old way or are they alongside the old way? If they replace the old way, then I why is no code deleted?

This comment has been minimized.

Copy link
@fariza

fariza Jan 24, 2018

Author Member

The new toolmanager and tools are set to replace the old navigationtoolbar. That is why we have to "repeat" some code
We can not just delete the old one, because many users play with the internals of navigationtoolbar. So for compatibility reasons we have to keep the old code there until it is fully deprecated

This comment has been minimized.

Copy link
@jklymak

jklymak Jan 24, 2018

Contributor

OK, but if I use Qt5Agg now, w/o mucking with internals I will get these widgets?

This comment has been minimized.

Copy link
@fariza

fariza Jan 24, 2018

Author Member

@jklymak if you look at the example, the only thing you have to do is

import matplotlib
matplotlib.use('QT5Agg')
matplotlib.rcParams['toolbar'] = 'toolmanager'

at the begining of your code

This comment has been minimized.

Copy link
@jklymak

jklymak Jan 24, 2018

Contributor

OK, I figured it out - toolmanager2 is still the default, and this only works if toolmanager is called.

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

button = QtWidgets.QToolButton(self)
button.setIcon(self._icon(image_file))

This comment has been minimized.

Copy link
@jklymak

jklymak Jan 24, 2018

Contributor
  • These don't seem to have hi-dpi enabled
  • Whatever the lightning bolt thing is, is not implimented.

figure_1_and_mep22_implementation_for_qt_backend_by_fariza_ _pull_request__9934_ _matplotlib_matplotlib 2

figure_1_and_4__python_testplot_py__python3_6__and_1__jklymak_valdez____leewaves17_lwregrid2neweb_and_7__jklymak_valdez____leewaves17_archivedruns_lwregrid2_python__zsh__and_testplot_py_ ___matplotlib

This comment has been minimized.

Copy link
@fariza

fariza Jan 24, 2018

Author Member

For the hi-dpi I have to check, but for the lightning bolt thing, this is normal. One of the main ideas behind MEP22 and MEP27 is to homogenize the tools and what is available in each backend.
Right now that tool is only available on QT with the navigationtoolbar

This PR is just to implement the basics of MEP22 to QT
Later another PR can add that tool for QT or preferably for all the backends

This comment has been minimized.

Copy link
@jklymak

jklymak Jan 24, 2018

Contributor

Fo hi dpi, NavigationToolbar2QT has some logic associated with is_pyqt5() that switches to '_large.png and calls setIconSize.

This comment has been minimized.

Copy link
@fariza

fariza Jan 24, 2018

Author Member

Here the logic is the same, we use a _icon_extension attribute in backend_bases.ToolContainerBase.
In the case of qt it gets replaced with a property

@property
 def _icon_extension(self):
        if is_pyqt5():
            return '_large.png'
        return '.png'

I have to carefully check this. But you are right, this can be done in a separate PR, because it might affect all the backends and not just this one.

@fariza

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2018

@jklymak
Copy link
Contributor

left a comment

I'm approving and if its hard to work on, maybe the HiDpi work can be done after this is merged in a separate PR. Its certainly usable at HiDpi, just doesn't look great.

plt.show()

The main example `examples/user_interfaces/toolmanager_sgskip.py` shows more
details, just adjust the header to use QT instead of GTK3

This comment has been minimized.

Copy link
@jklymak

jklymak Jan 29, 2018

Contributor

As per the call, this probably needs to be explicitly marked as an experimental API so we can change it w/o having to go through deprecation cycles.

This comment has been minimized.

Copy link
@fariza

fariza Jan 31, 2018

Author Member

@jklymak I don't understand your comment, as soon as you run, it shows a warning about experimental status

This comment has been minimized.

Copy link
@jklymak

jklymak Jan 31, 2018

Contributor

Sure, and the whats-new should make this explicit as well.

Federico Ariza
@fariza

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2018

@jklymak I added the warning text in the whats new

@tacaswell tacaswell merged commit 2e83a8a into matplotlib:master Feb 5, 2018

5 of 8 checks passed

codecov/patch 26.01% of diff hit (target 50%)
Details
codecov/project/tests 98.81% (-0.05%) compared to 2f28286
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci: docs-python27 Your tests passed on CircleCI!
Details
ci/circleci: docs-python35 Your tests passed on CircleCI!
Details
codecov/project/library 63.69% (target 50%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
lgtm analysis: Python No alert changes
Details
@tacaswell

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

Thanks @fariza ! Glad we can move this along.

@QuLogic QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.