search

elm-community / SublimeElmLanguageSupport

Elm language syntax highlighting and tool integration for ST2/3.
https://packagecontrol.io/packages/Elm%20Language%20Support
MIT License
33 stars 12 forks source link

Suppress console flash on Windows #59

Closed ianmackenzie closed 5 years ago

ianmackenzie commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Released in 1.0.0 beta 4. Thanks, @ianmackenzie!