MEP22 implementation for QT backend #9934

fariza · 2017-12-05T16:29:59Z

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()

New Toolbar and QT specific tools

fariza · 2017-12-05T16:32:52Z

@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

anntzer · 2017-12-05T16:40:41Z

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

fariza · 2017-12-05T17:05:57Z

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

anntzer · 2017-12-05T19:07:53Z

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

OceanWolf · 2017-12-05T21:49:29Z

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 · 2018-01-04T22:32:54Z

@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 · 2018-01-04T23:01:10Z

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

tacaswell · 2018-01-05T00:06:12Z

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

jklymak · 2018-01-24T17:49:04Z

lib/matplotlib/backends/backend_qt5.py

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


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

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

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

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

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?

jklymak · 2018-01-24T17:53:34Z

lib/matplotlib/backends/backend_qt5.py

+        del self._toolitems[name]
+
+
+class StatusbarQt(StatusbarBase, QtWidgets.QLabel):


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?

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

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

@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

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

jklymak · 2018-01-24T18:31:08Z

lib/matplotlib/backends/backend_qt5.py

+            self, name, group, position, image_file, description, toggle):
+
+        button = QtWidgets.QToolButton(self)
+        button.setIcon(self._icon(image_file))


These don't seem to have hi-dpi enabled

Whatever the lightning bolt thing is, is not implimented.

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

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

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 · 2018-01-24T19:33:59Z

No. The statusbar is where the coordinates of the.cursor appears

…

On Jan 24, 2018 2:06 PM, "Jody Klymak" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/matplotlib/backends/backend_qt5.py <#9934 (comment)> : > if self.toolbar is not None: self.window.addToolBar(self.toolbar) - self.toolbar.message.connect(self.statusbar_label.setText) + if not self.toolmanager: + # add text label to status bar + statusbar_label = QtWidgets.QLabel() 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? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9934 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABa86b07FEnc86Q6SS8ZFy1N2SsDuokQks5tN39GgaJpZM4Q2k3F> .

jklymak

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.

jklymak · 2018-01-29T21:14:51Z

doc/users/next_whats_new/qt-toolmanager.rst

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


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.

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

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

fariza · 2018-02-01T13:40:46Z

@jklymak I added the warning text in the whats new

tacaswell · 2018-02-05T21:02:24Z

Thanks @fariza ! Glad we can move this along.

MEP22 implementation for QT backend

a3b049f

New Toolbar and QT specific tools

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

fix qt5

958b512

fariza mentioned this pull request Jan 4, 2018

refactor non wx.ToolBar specific parts of NavigationToolbar2Wx/Agg into NavigationController2Wx/Agg #10145

Closed

6 tasks

tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jan 5, 2018

jklymak reviewed Jan 24, 2018

View reviewed changes

jklymak approved these changes Jan 24, 2018

View reviewed changes

jklymak reviewed Jan 29, 2018

View reviewed changes

warning in whats new

943c40d

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

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

		del self._toolitems[name]


		class StatusbarQt(StatusbarBase, QtWidgets.QLabel):

Uh oh!

MEP22 implementation for QT backend #9934

MEP22 implementation for QT backend #9934

Uh oh!

Conversation

fariza commented Dec 5, 2017

PR Summary

PR Checklist

To test it

Uh oh!

fariza commented Dec 5, 2017

Uh oh!

anntzer commented Dec 5, 2017

Uh oh!

fariza commented Dec 5, 2017

Uh oh!

anntzer commented Dec 5, 2017

Uh oh!

OceanWolf commented Dec 5, 2017

Uh oh!

fariza commented Jan 4, 2018

Uh oh!

anntzer commented Jan 4, 2018

Uh oh!

tacaswell commented Jan 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fariza commented Jan 24, 2018 via email

Uh oh!

jklymak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fariza commented Feb 1, 2018

Uh oh!

tacaswell commented Feb 5, 2018

Uh oh!

Uh oh!