8000 Fix for issue4372 by mdehoon · Pull Request #4452 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Fix for issue4372 #4452

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

Merged
merged 4 commits into from
Jun 1, 2015
Merged

Fix for issue4372 #4452

merged 4 commits into from
Jun 1, 2015

Conversation

mdehoon
Copy link
Contributor
@mdehoon mdehoon commented May 21, 2015

This adds a check at run-time to make sure Python is installed as a framework. This will avoids problems with non-framework Pythons as seen in issue #4372 .

@tacaswell tacaswell added this to the next point release milestone May 21, 2015
@WeatherGod
Copy link
Member

Is there a reference that could be added as a comment that would support this logic?

@jenshnielsen
Copy link
Member

I tried this out. Im not sure that this check is strict enough. As far as I understand a default virtual env build is not a framework however this function returns True:

Running the following with python 2.7.9 (Homebrew) On OSX 10.10

import sys
import MacOS

print sys.version
print "Is WMAvailable: " + str(MacOS.WMAvailable())

from matplotlib.backends import _macosx
print "According to verify_main_display: " + str(_macosx.verify_main_display())

Gives:

2.7.9 (default, Jan 29 2015, 06:27:40)
[GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.56)]
Is WMAvailable: False
According to verify_main_display: True

IE the results from verify_main_display and WMAvailable are different.

@mdehoon
Copy link
Contributor Author
mdehoon commented May 24, 2015

@jenshnielsen Thanks for checking. That is really weird, because the code for verify_main_display and WMAvailable are essentially the same. Do you know which one is correct? If you run

>>> from pylab import *
>>> figure()

do you get two active windows (the terminal and the figure), or only one? You can compare to the screenshots shown in issue #4372 .

@jenshnielsen
Copy link
Member

I am fairly sure WMAvailable() is correct.

Running python testframework.py where testframework.py contains

from pylab import *
figure()
show()

Outside a virtual env the focus is on the figure and not the terminal.
In the virtual env the focus remains on the terminal.

Related notes.

WXPython (Tested with the new WX Phoenix prerelease) has a similar check which prevents the code from running inside a virtual env.

This program needs access to the screen. Please run with a
Framework build of python, and only when you are logged in
on the main display of your Mac.

There are several resources available online documenting the issues with frameworks and virtualenv

I.E. http://2012-2013.chriskrycho.com/web/python-homebrew-virtualenv-os-x-framework-builds/
and http://wiki.wxpython.org/wxPythonVirtualenvOnMac

@jenshnielsen
Copy link
Member

BTW the code from WX that seems to work is in https://svn.wxwidgets.org/svn/wx/wxPython/Phoenix/trunk/src/app_ex.cpp IsDisplayAvailable

Looks like it also gets the serial number and tries to foreground the process.

Modifying the code to match that seems to get the correct result. However both
GetCurrentProcess and SetFrontProcess are deprecated so presumably we should use something else

verify_main_display(PyObject* self)
{
    PyObject* result;
    ProcessSerialNumber psn;
    CGDirectDisplayID display = CGMainDisplayID();
    if (display == 0) result = Py_False;
    else {
        if (GetCurrentProcess(&psn) < 0 || SetFrontProcess(&psn) < 0) {
            result = Py_False;
        } else {
            result = Py_True;
        }
    }
    Py_INCREF(result);
    return result;
}

@mdehoon
Copy link
Contributor Author
mdehoon commented May 24, 2015

@jenshnielsen Great, thanks! My only concern here is that GetCurrentProcess and SetFrontProcess are from Carbon, not from Cocoa. Can you have a look at what happens in the launch function in src/_macosx.m? This function gathers the ProcessSerialNumber the Cocoa way. I am wondering if we get a reasonable value for the ProcessSerialNumber in the launch function if Python is not a framework.

@jenshnielsen
Copy link
Member

I have submitted a pr against your branch with a port to Cocoa. I am however by no means a Cocoa or Objective-C expert so it is possible that I am doing it wrong. Any advise is welcomed

mdehoon#1

@jenshnielsen
Copy link
Member

Wait. That doesn't seem to work. The result depends on whether WMAvailable() has been called before hand in the same process. 😞

@jenshnielsen
Copy link
Member

Adding a call to GetCurrentProcess before using the Cocoa api fixes the issue (Which is what WMAvailable() ) effectively does . But I cannot figure out a way to do it only with the Cocoa API.

Without calling GetCurrentProcess

  • [[NSRunningApplication currentApplication] activationPolicy];
  • [[NSRunningApplication currentApplication] processIdentifier];

both return -1

After calling GetCurrentProcess they return useful values.

Trying to activate the application with [[NSRunningApplication currentApplication] activateWithOptions: NSApplicationActivateIgnoringOtherApps]; Including other possible options do no difference.

Any ideas?

@jenshnielsen
Copy link
Member

Ok I have a working solution in mdehoon#1

An app needs to be launched before we can get the activation policy.

I am not at all sure this is the best solution so I am interested in any feedback.

@mdehoon
Copy link
Contributor Author
mdehoon commented May 27, 2015

The latest pull request is based on the solution suggested by @jenshnielsen , but uses the Cocoa code when available, and the Carbon code otherwise.

case NSApplicationActivationPolicyAccessory:
return true;
case NSApplicationActivationPolicyProhibited:
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a default case handling -1? I.e. when this is called without an NSApplication? Perhaps raising a warning that the check will not work? Alternatively I think that we should at least document that this will not work without an NSApplication

@jenshnielsen
Copy link
Member

Looks great. This is now a runtime error rather than a warning. Any particular reason?

I am not sure what to prefer? On on hand a plot with basic functionality is better than no plot.
On the other hand functionality is broken in random ways and perhaps it is better to just explicitly error out.

I will try to write something up for the documentation explaining how to get a framework build in a virtual env.

@mdehoon
Copy link
Contributor Author
mdehoon commented May 27, 2015

Could we add a default case handling -1? I.e. when this is called without an NSApplication? Perhaps
raising a warning that the check will not work? Alternatively I think that we should at least document
that this will not work without an NSApplication

The verify_framework function is now an internal function, which is called from init_macosx but is otherwise not exported. The verify_framework function is called after calling [NSApplication sharedApplication], so the NSApplication is guaranteed to exist.

Looks great. This is now a runtime error rather than a warning. Any particular reason?

In matplotlib 1.4.3, it actually was a compile-time error. Also in the current version of _macosx.m, Python being installed as a framework is more critical than previously; if Python is not a framework, the event loop will seriously hang (see the new launch: function in _macosx.m, which depends on getting the process serial number). As framework problems are hard to track down, I would prefer a hard error than just a warning.

@jenshnielsen
Copy link
Member

@mdehoon Yes I realise that is now an internal function. I just think it's a good idea to document it since it is not at all obvious that an NSApplication must exist beforehand and someone in the future might be tempted to refactor this to make it a public function.

Im fine with making it a hard error.

@mdehoon
Copy link
Contributor Author
mdehoon commented Jun 1, 2015

If there are no major objections, can this pull request be accepted?

@jenshnielsen
Copy link
Member

OK, I really still don't understand why you don't want to document that the routine cannot be called but without an nsapp. The fact that it can not happen in the present code is not enough to ensure that it can't happen in a refactored version but whatever.

@jenshnielsen jenshnielsen reopened this Jun 1, 2015
jenshnielsen added a commit that referenced this pull request Jun 1, 2015
@jenshnielsen jenshnielsen merged commit 0a4fe6d into matplotlib:master Jun 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0