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

:apple: skip tabbing category if macOS < Sierra #10696

Merged
merged 2 commits into from Oct 17, 2017

Conversation

Projects
None yet
9 participants
@primalmotion
Contributor

primalmotion commented Oct 5, 2017

Previously, the macro was ensuring theMAC_OS_X_VERSION_10_12 was not defined to decide to compile a NSWindow category back porting native tabs or not.

This patch ensures to compile the NSWindow category only if the min required version is lesser than 1012 (MAC_OS_X_VERSION_10_12)

fixes #10657

🍎 skip tabbing category if macOS < Sierra
Previously, the macro was ensuring the` MAC_OS_X_VERSION_10_12` was not defined to decide to compile a `NSWindow` category back porting native tabs or not.

This patch ensures to compile the `NSWindow` category only if the min required version is lesser than 1012 (`MAC_OS_X_VERSION_10_12`)

@primalmotion primalmotion requested a review from electron/reviewers as a code owner Oct 5, 2017

@welcome

This comment has been minimized.

Show comment
Hide comment
@welcome

welcome bot Oct 5, 2017

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.
    We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

welcome bot commented Oct 5, 2017

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.
    We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.
@primalmotion

This comment has been minimized.

Show comment
Hide comment
@primalmotion

primalmotion Oct 5, 2017

Contributor

Checks actually failed where they should not have. Let me check on it

Contributor

primalmotion commented Oct 5, 2017

Checks actually failed where they should not have. Let me check on it

@codebytere

looks good, thanks for adding this!

@NotAmaan

This comment has been minimized.

Show comment
Hide comment
@NotAmaan

NotAmaan Oct 16, 2017

When will this be merged?

NotAmaan commented Oct 16, 2017

When will this be merged?

@jkleinsc jkleinsc merged commit cbda307 into electron:master Oct 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@welcome

This comment has been minimized.

Show comment
Hide comment
@welcome

welcome bot Oct 17, 2017

Congrats on merging your first pull request! 🎉🎉🎉

welcome bot commented Oct 17, 2017

Congrats on merging your first pull request! 🎉🎉🎉

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Oct 17, 2017

Contributor

@primalmotion @codebytere could you verify this works? I am running VS Code with a version that includes this patch and see no difference. Interestingly when I click into the space on top of the window (where the tabs are), a new window opens. It actually behaves as if the area there was the "+" button for opening a new tab:

flicker_chrome58

Contributor

bpasero commented Oct 17, 2017

@primalmotion @codebytere could you verify this works? I am running VS Code with a version that includes this patch and see no difference. Interestingly when I click into the space on top of the window (where the tabs are), a new window opens. It actually behaves as if the area there was the "+" button for opening a new tab:

flicker_chrome58

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Oct 17, 2017

Member

Looking at this code, not sure how it would actually fix it (those forward declarations wouldn't affect respondsToSelector). Seeing strange things like @bpasero, I'll investigate this tomorrow

Member

MarshallOfSound commented Oct 17, 2017

Looking at this code, not sure how it would actually fix it (those forward declarations wouldn't affect respondsToSelector). Seeing strange things like @bpasero, I'll investigate this tomorrow

@MarshallOfSound MarshallOfSound self-assigned this Oct 17, 2017

@primalmotion

This comment has been minimized.

Show comment
Hide comment
@primalmotion

primalmotion Oct 17, 2017

Contributor
Contributor

primalmotion commented Oct 17, 2017

@primalmotion

This comment has been minimized.

Show comment
Hide comment
@primalmotion

primalmotion Oct 17, 2017

Contributor

@MarshallOfSound it's not the perfect fix, as if you build Electron a a macOS < Sierra, the category will still be compiled, leading to this behavior (I think). This patch just makes sure that building it on High Sierra doesn't compile the category. Previously the category was skipped only on Sierra.

Contributor

primalmotion commented Oct 17, 2017

@MarshallOfSound it's not the perfect fix, as if you build Electron a a macOS < Sierra, the category will still be compiled, leading to this behavior (I think). This patch just makes sure that building it on High Sierra doesn't compile the category. Previously the category was skipped only on Sierra.

@haad

This comment has been minimized.

Show comment
Hide comment
@haad

haad Oct 30, 2017

I did it myself and it works great :)

haad commented Oct 30, 2017

I did it myself and it works great :)

@primalmotion primalmotion deleted the primalmotion:patch-1 branch Nov 22, 2017

@linmodev

This comment has been minimized.

Show comment
Hide comment
@linmodev

linmodev Nov 25, 2017

So the latest version of VS Code is still not working properly?

linmodev commented Nov 25, 2017

So the latest version of VS Code is still not working properly?

@haad

This comment has been minimized.

Show comment
Hide comment
@haad

haad Nov 28, 2017

Yes it's still doesn't work properly.

haad commented Nov 28, 2017

Yes it's still doesn't work properly.

@phoenixgao

This comment has been minimized.

Show comment
Hide comment
@phoenixgao

phoenixgao Dec 18, 2017

@primalmotion Could you share your build? :D I trust you

phoenixgao commented Dec 18, 2017

@primalmotion Could you share your build? :D I trust you

@haad

This comment has been minimized.

Show comment
Hide comment
@haad

haad Dec 18, 2017

Electron build was really easy just clone it and follow README.

haad commented Dec 18, 2017

Electron build was really easy just clone it and follow README.

@primalmotion

This comment has been minimized.

Show comment
Hide comment
@primalmotion

primalmotion Jan 24, 2018

Contributor

@phoenixgao there you go:

https://we.tl/jDjfMDfGVb

But you really must trust me :)

Contributor

primalmotion commented Jan 24, 2018

@phoenixgao there you go:

https://we.tl/jDjfMDfGVb

But you really must trust me :)

@linmodev

This comment has been minimized.

Show comment
Hide comment
@linmodev

linmodev Feb 7, 2018

@primalmotion The file is deleted

linmodev commented Feb 7, 2018

@primalmotion The file is deleted

@phoenixgao

This comment has been minimized.

Show comment
Hide comment
@phoenixgao

phoenixgao Feb 7, 2018

@linmodev I uploaded primalmotion's build to github, you can check here
https://github.com/phoenixgao/electron-high-sierra-build

But you must trust him and me :D

phoenixgao commented Feb 7, 2018

@linmodev I uploaded primalmotion's build to github, you can check here
https://github.com/phoenixgao/electron-high-sierra-build

But you must trust him and me :D

@linmodev

This comment has been minimized.

Show comment
Hide comment
@linmodev

linmodev Feb 7, 2018

@phoenixgao Thanks! It is already working.

linmodev commented Feb 7, 2018

@phoenixgao Thanks! It is already working.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Jun 9, 2018

Contributor

I am no longer convinced this was ever or is still a problem with Electron (see Microsoft/vscode#35361 (comment)).

Contributor

bpasero commented Jun 9, 2018

I am no longer convinced this was ever or is still a problem with Electron (see Microsoft/vscode#35361 (comment)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment