Skip to content
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

Suppress console flash on Windows #59

Conversation

ianmackenzie
Copy link

Finally got around to investigating #50 and found out that there's a (Windows-specific) bit of functionality in Python's Popen constructor to suppress the console window. Not entirely sure why this wasn't an issue with the 0.18 version - was shell=True used before, perhaps? According to https://docs.python.org/3.3/library/subprocess.html#subprocess.STARTUPINFO, passing shell=True will implicitly cause SW_HIDE to be set (but using shell=True is "strongly discouraged" for security reasons, so it's better to configure startupinfo explicitly).

This change solves the issue for me on Windows (and seems to make running elm make a little bit faster, as a bonus). I don't currently have a Mac or Linux machine, so this has not yet been tested to make sure that it doesn't cause problems on other OSes (the if os.name == 'nt' should take care of that, but I may have messed something up).

@ianmackenzie
Copy link
Author

I'm not entirely sure what the 'right' way to check for Windows is here: os.name == 'nt' seemed like the most common, but sys.platform == 'win32' and platform.system() == 'Windows' are also contenders. Might be an issue if Sublime Text is launched from Cygwin or something?

@sentience
Copy link

Thanks @ianmackenzie! I'll give it a quick test on OS X tonight, and if it doesn't cause a problem there I'll go ahead and release this as a new beta. I suspect you're right that shell=True was previously in use.

@sentience
Copy link

sentience commented Jan 11, 2019

@ianmackenzie Still seems to work well on macOS, so I'm happy to merge this.

Given that we're also using Popen in elm_format.py (https://github.com/ianmackenzie/SublimeElmLanguageSupport/blob/fa3cf79d20a42603b40f589149bf6f62c7780703/elm_format.py#L31-L31), should we be making the same change for that? Looks like it's currently using the shell=True hack.

@ianmackenzie
Copy link
Author

Good point! I've refactored things a bit to use startupinfo for elm-format as well; see what you think. I can confirm that removing shell=True from elm_format.py does result in a flashing console window when running elm-format, and then adding startupinfo suppresses it again.

@ianmackenzie
Copy link
Author

By the way, some interesting discussion on os.name vs sys.platform vs platform.system(): https://stackoverflow.com/questions/4553129/when-to-use-os-name-sys-platform-or-platform-system/11674977

I think os.name is probably fine for now, but if that turns out to be too broad then sys.platform should be somewhat finer-grained.

@sentience sentience merged commit 7244b95 into elm-community:elm-0.19 Jan 22, 2019
@sentience
Copy link

Released in 1.0.0 beta 4. Thanks, @ianmackenzie!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants