-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Fix for issue4372 #4452
Conversation
…if Python was installed as a framework
Is there a reference that could be added as a comment that would support this logic? |
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:
IE the results from verify_main_display and WMAvailable are different. |
@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 . |
I am fairly sure WMAvailable() is correct. Running from pylab import *
figure()
show() Outside a virtual env the focus is on the figure and not 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.
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/ |
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
|
@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. |
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
|
Wait. That doesn't seem to work. The result depends on whether WMAvailable() has been called before hand in the same process. 😞 |
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
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? |
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. |
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; |
There was a problem hiding this comment.
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
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. I will try to write something up for the documentation explaining how to get a framework build in a virtual env. |
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.
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. |
@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. |
If there are no major objections, can this pull request be accepted? |
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. |
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 .