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

exec node: add windowsHide option #3026

Merged
merged 4 commits into from
Jul 2, 2021

Conversation

natcl
Copy link
Contributor

@natcl natcl commented Jun 16, 2021

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Proposed changes

This adds a Hide windows checkbox to the exec node to allow hiding shell windows under windows. Without this, the windowsHide option of exec and spawn defaults to false and shell windows will appear briefly when running commands.

Checklist

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the forum/slack team.
  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

@@ -80,4 +80,5 @@ <h4>Killing processes</h4>
<p>If the node has more than one process running then <code>msg.pid</code> must also be set with the value of the PID to be killed.</p>
<p>If a value is provided in the <code>Timeout</code> field then, if the process has not completed when the specified number of seconds has elapsed, the process will be killed automatically</p>
<p>Tip: if running a Python app you may need to use the <code>-u</code> parameter to stop the output being buffered.</p>
<p>The <code>Hide windows</code> option can be set to hide shell windows under Windows.</p>
Copy link
Member

Choose a reason for hiding this comment

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

should we emphasis that this ONLY has an effect under Windows ? or does it do something under Linux/Mac also ?

@dceejay
Copy link
Member

dceejay commented Jun 18, 2021

May just need to make clear this an option for Windows only - and check it doesn't do bad things if set on other OS. - but yes looks good.

@natcl
Copy link
Contributor Author

natcl commented Jun 18, 2021

Hi Dave, just added a clarification, let me know if it suits you !
Thanks !

@Hugobox
Copy link
Contributor

Hugobox commented Jun 29, 2021

Tested on my windows PC, works well! Thanks! Looking forward this merge!

@dceejay
Copy link
Member

dceejay commented Jun 29, 2021

All good with me. @knolleary happy for you to merge for v2

@knolleary
Copy link
Member

Thanks for this. Will merge, but am going to modify the wording slightly... says "Windows" a few too many times in a single sentence :)

@knolleary knolleary merged commit 04f4a76 into node-red:dev Jul 2, 2021
@knolleary
Copy link
Member

Summary of changes:

  • renamed the property to 'winHide' for brevity
  • relabelled 'hide console' and updated the help text

@natcl
Copy link
Contributor Author

natcl commented Jul 2, 2021

Thanks !!

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.

None yet

4 participants