Qt5: Move agg draw to main thread and fix rubberband

[pwuertz]

Follow up from #4962

[tacaswell]

This needs a rebase, sorry.

[pwuertz]

This PR contains two fixes.

The first one moves the agg draw back to the main thread. Qt uses a dedicated render thread for paintEvent. Although the agg buffer should be updated as closely to the qt repaint as possible in order to take all the latest events into account, accessing the scenegraph from the render thread is dangerous since the scenegraph has no locking mechanism.

The second and third commit address problems with the rubberband in qt. The second adds a remove_rubberband() handler to backend_bases that is invoked when the zoom rectangle is released.

The last commit changes the rubberband handling in the qt backend to utilize the rubberband removal handler. The previous solution was to simply remove the rectangle after each draw. This method fails when there is an additional paintEvent between two draw_rubberband() calls because it clears the rubberband while it is still being dragged (flicker). Also, there is the chance that the paintEvent requested by the last draw_rubberband() call before releasing the mouse might get delayed until after the draw_idle() call for zooming in. This causes the rubberband to be drawn on top of the zoomed-in figure (bad) and since there are no more events after the release the rubberband will stay on screen (worse).
Also, changing the state of the rubberband doesn't trigger an agg buffer redraw anymore, freeing some CPU cycles while dragging.

[pwuertz]

@jrevans Do you have an example for the situation you described where paintEvent is called while self.renderer doesn't exist yet?

[tacaswell]

My reading of @jrevans comments is that was his guess as to why the draw was there. That conditional can probably be removed.

[tacaswell]

I would like confirmation from @jrevans that this does not break any of their use cases.

I gave this a read over and it seems reasonable, but need sink more time into it. I am slightly worried about the MI re-ordering and making sure all of our classes are correctly cooperating when they need to be (one of the big changes between pyQt4 and pyQt5 is that their classes are now cooperating) so everything works with both qt4 and qt5.

attn @mfitzp (you clearly need a distraction from thesis writing 😈 )

[tacaswell]

@astrofrog Can you have a look at how this impacts glueviz?

[astrofrog]

@tacaswell - we don't yet use Qt5 in glueviz, so I'm not able to test this yet (unless this impacts Qt4 in some way?)

[tacaswell]

It does, qt4 is implemented as a shim on to of qt5.

On Sun, Aug 23, 2015, 11:27 AM Thomas Robitaille notifications@github.com
wrote:

@tacaswell https://github.com/tacaswell - we don't yet use Qt5 in
glueviz, so I'm not able to test this yet (unless this impacts Qt4 in some
way?)

—
Reply to this email directly or view it on GitHub
#4972 (comment)
.

[astrofrog]

@tacaswell - ok, in that case I will test this out tomorrow with glueviz

[astrofrog]

@tacaswell - sorry for the delay. Unfortunately this does break glueviz:

Before (master):

After (this PR):

[pwuertz]

Any idea why this would have an impact on glueviz while the regular gui operates normally?

[tacaswell]

glueviz does some monkey patching to try and control the draw cadence (if I recall correctly). I suspect that someplace they are calling draw instead of draw_idle.

[pwuertz]

Strange, that shouldnt stop the Widget from painting. Anything hardcoded to the order of the base and Mixin-classes? I will probably look into installing glueviz also...

[tacaswell]

I worry about this change a bit.

[pwuertz]

Indeed, both FigureCanvasQT and FigureCanvasQTAggBase define draw() methods, and the one from FigureCanvasQT is supposed to be overwritten. So FigureCanvasQTAggBase must be the first class for now.

[pwuertz]

Yea, seems to be a problem with the python MRO. The glueviz problem is caused by making FigureCanvasQT the first class instead of FigureCanvasQTAggBase, although making the QObject the first class this is the preferred layout according to the PyQt documentation.

[pwuertz]

Reverted to original class order and rebased to current master.

[tacaswell]

@astrofrog @jrevans Can you both please check the impact of this branch on your applications?

[tacaswell]

Do any other backends implement this?

[pwuertz]

Not at this time. I found a class named RubberbandBase in backend_tools.py that looks like a template for rubberband functionality, including a remove_rubberband function. Gtk3 and TK seem to implement this class. However, this code path is never used. Seems to me that this solution has been abandoned at some time and now only draw_rubberband implemented by the canvas is used.

[tacaswell]

Other way around, the toolbase work is functionality that is just starting to be built out by @fariza and @OceanWolf

[fariza]

@pwuertz It's not abandoned, It's just the beginning. For now only Gtk3 and Tk are implemented, we are waiting for MEP27 to land to finish the other backends

[pwuertz]

Ah, well then porting this solution to the toolbase framework should be straight forward since it implements the same draw/remove concept.

[OceanWolf]

Yes, the codepath does get used see FigureManagerGTK3 and FigureManagerTkAgg which calls this codepath via backend_tools.default_tools. In the upcoming 1.5 release this will go out as an experimental proof of concept, then we will begin work on the final polishing, MEP27 does this a bit (see #4143), and other commits related to MEP22 (see #3652). So for 2.1 we should have this ready to go out fully.

Feel free to test, just set rcParam['toolbar'] = 'toolmanager', but as we say, only on GTK3 and TKAgg for now.

[astrofrog]

@tacaswell @pwuertz - just confirming that this now works with glueviz