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 notification actions not working on High Sierra #10709

Merged
merged 1 commit into from Oct 17, 2017

Conversation

Projects
None yet
5 participants
@MarshallOfSound
Member

MarshallOfSound commented Oct 7, 2017

Fixes #10655

@MarshallOfSound MarshallOfSound requested a review from electron/notifications as a code owner Oct 7, 2017

@MarshallOfSound MarshallOfSound requested a review from electron/reviewers Oct 15, 2017

@codebytere

Looks good to me pending that one check that's failing :)

@zcbenz

zcbenz approved these changes Oct 17, 2017

@zcbenz zcbenz merged commit 4dc7477 into master Oct 17, 2017

7 of 8 checks passed

electron-mas-x64 Build #5342 failed in 16 min
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-osx-x64 Build #5329 succeeded in 13 min
Details

@zcbenz zcbenz deleted the fix-notification-actions-hs branch Oct 17, 2017

@ckerr

This comment has been minimized.

Member

ckerr commented Oct 17, 2017

Should be good for the first two million notifications or so 😄

Did it make sense to merge this with the electron-mas-x64 build failing though? Is that a false negative?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Oct 17, 2017

@ckerr Yeah, if you're showing more than 2 million notifications you're gonna have bigger problems than this int overflowing 😆

The MAS build failed on webview tests which are unrelated to this code change so called out as a flake.

@ilyaLibin

This comment has been minimized.

ilyaLibin commented Jan 23, 2018

@MarshallOfSound Still doesn't work for me. :/
Same code
"electron": "^1.7.10",
OS High Sierra 10.13.2

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jan 23, 2018

@ilyaLibin This fix is not in the 1.7.x series, only in the 1.8.x series

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