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

fix(windows): electron window not displaying #180

Merged
merged 1 commit into from Jan 8, 2021

Conversation

zoltan-mihalyi
Copy link
Contributor

@zoltan-mihalyi zoltan-mihalyi commented Jan 5, 2021

Platforms affected

  • Windows (electron window is hidden)
  • others (stdio is ignored)

Motivation and Context

On windows platform using nodejs 11+, the electron window does not appear.

Description

windowsHide: false makes the electron window appear on windows.

Testing

I manually tested my changes on windows.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@erisu
Copy link
Member

erisu commented Jan 8, 2021

Hey @zoltan-mihalyi,

Thank you for the PR.

I have configured my Windows environment and tested the before and after case to confirm the original issue and fix.

I would like to actually split this PR into two PRs. I know this PR is small and only changes one line, but IMO this PR is doing two things, a fix and a feature.

You can make the second PR based from the first. You or I can rebasae after we merge in the first fix PR.

First PR (Fix)

Lets convert this PR into a fix only PR.

  • Change the PR title into something like this:

    fix(windows): electron window not displaying

  • Update the description to only apply for this fix.
  • Update the code to only apply the fix.

Second PR (Feature)

IMO, adding the stdio flag is more of a feature request and not apart of the fix.

Lets make a new PR for this feature request.

  • Create a PR title into something like this:

    feat: display electron’s process stdio in terminal

  • Set the description explaining the new feature, etc.
  • Add new feature code.

Overall

The changes are good and works..

@erisu erisu self-requested a review January 8, 2021 08:30
@erisu erisu added this to the 3.0.0 milestone Jan 8, 2021
@zoltan-mihalyi zoltan-mihalyi changed the title Fix Electron window is hidden in windows, stdio ignored fix(windows): electron window not displaying Jan 8, 2021
@zoltan-mihalyi
Copy link
Contributor Author

@erisu The first PR is done, I will create the second one after this is finished.

@erisu erisu merged commit eb2ee51 into apache:master Jan 8, 2021
@erisu
Copy link
Member

erisu commented Jan 8, 2021

@zoltan-mihalyi I just squashed merged the PR so you can update your fork copy and use the main branch to create the second PR from.

I will review & merge the second PR as soon as I see it ready & finishes running the tests.

@miroslavvojtus
Copy link

We have the same problem.
When we can expect the next version?

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

3 participants