backend_wx.py: ReleaseMouse() when the capture is lost #7652
Conversation
|
@Kojoley Can you look at those pytest failures? It looks like it is missing saving a files someplace? |
|
Test failures are un-related to this PR. |
|
@tacaswell appveyor failure ( |
|
This should not be merged. IIRC, capture is necessary to translate events, such as button_press_event, in matplotlib. e.g. what if a user presses the mouse button in a canvas, but drags outside before the button up happens? This PR seems like a huge hammer to kill off the assertions. The assertions are an indication that some coding is wrong, but that should not mean that the baby gets thrown out with the bathwater. 8000 |
My understanding is that the button up event, say I've had code that monkey-patches So, I would guess I would turn your question around: Having CaptureMouse() clearly causes some problems on some systems.... when does it ever provide desirable behavior? My view is that, at best, it has no effect. Does this PR break some behavior for you? Since
I believe backend_wx.py has been wrong for a very long time by capturing the mouse without a guarantee of it being released. I don't believe there are negative side effects ("baby") here, just negative behavior ("bathwater"). But, if there are unintended consequences, we should figure out how to solve them. |
|
@tacaswell @dhyams OK, I tested this more thoroughly and Linux, Mac OSX, and Windows. Removing So, I changed backend_wxp.py to return to doing the CaptureMouse() on the Mouse-Down events, and doing the conditional ReleaseMouse() on the Mouse-Up events, and added handling of the MOUSE_CAPTURE_LOST and MOUSE_CAPTURE_CHANGED events, which is also just the conditional ReleaseMouse(). This works for me on all platforms, though I did not exhaustively test wxPython versions, and was testing with MPL 1.5.*. I do not see any C++ assertion errors on Linux or OSX with this code with some playing around with plotting and imaging panels. Since the problem was somewhat intermittent anyway, I'm not certain that this is completely fixed. The alternative would be to only capture/release the mouse on Windows. |
|
Please rebase instead of merging master (and preferably just keep the one commit of new changes instead of delete and revert, if possible.) |
This PR now does the opposite of what you reviewed.
Current coverage is 62.06% (diff: 45.00%)@@ master #7652 diff @@
==========================================
Files 174 174
Lines 56007 56027 +20
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 34765 34773 +8
- Misses 21242 21254 +12
Partials 0 0
|
They appear as one giant blob right now since they're bytes.
|
Something has gone very funny here with git.... |
|
Probably due to using $ git status
# Check that there's nothing you want to keep.
$ git checkout master
$ git fetch upstream
$ git rebase -i upstream/master@newville should get only the 3 commits you've originally added listed in your editor. Change the second two lines from $ git push --force origin master(and in the future, make sure to check out a new branch before starting any work!) |
|
@tacaswell @QuLogic Yes, sorry, the original changes and description for this PR were not sufficient to solve the problem described in #7636 and wxmplot/issue/25 and broke desirable behavior on Windows. I've updated my branch a couple times as I've learned about the nuances of how wx mouse events differ across platforms (see also https://groups.google.com/forum/#!topic/wxpython-users/-NrlWBMNkwM). At this point, I believe I have a backend_wx.py that will work (supporting the desirable behavior while avoiding the C++ assertion errors) everywhere, but I will not be able to test sufficiently on all platforms until next week. I'll close this PR and create another with a single commit on a freshly forked repo by the end of next week. |
This removes all calls to
wxWindow.CaptureMouse(), wxWindow.ReleaseMouse()and wxWindow.HasCapture()for the wx backend.This is intended to address C++ assertion errors as described in #7636 and which appear to happen when the window captures the mouse and then loses focus.
My view is that the window shouldn't be trying to capture the mouse (which is intended to prevent it transferring to other windows or apps) at all. I believe none of the other backends try to do this. More importantly, the capture of the mouse is not being handled properly, because the LoseFocus events are not being handled.