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

feat: add object to subscribe notification callback #19110

Conversation

@louismrose
Copy link
Contributor

louismrose commented Jul 4, 2019

Description of Change

Closes #19097.

This change exposes the object property on NSNotification to the callback passed to Electron's systemPreferences.subscriptionNotification methods.

I'm cc-ing @zcbenz on this PR as he worked on a very similar feature under #5582.

Checklist

Release Notes

Notes: Exposed the value of NSNotification.object to subscribers of notifications in systemPreferences.

@welcome

This comment has been minimized.

Copy link

welcome bot commented Jul 4, 2019

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR 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.

@louismrose

This comment has been minimized.

Copy link
Contributor Author

louismrose commented Jul 4, 2019

I'm not too sure why these builds are failing -- I don't think it's related to my changes, but I might be missing something obvious! Any tips from a contributor would be greatly appreciated? I'm happy to tackle any further changes needed here.

@electron-cation electron-cation bot removed the new-pr 🌱 label Jul 5, 2019
docs/api/system-preferences.md Outdated Show resolved Hide resolved
@louismrose

This comment has been minimized.

Copy link
Contributor Author

louismrose commented Jul 8, 2019

Thanks for the feedback @zcbenz. I've made the documentation change that you suggested.

@zcbenz
zcbenz approved these changes Jul 9, 2019
@louismrose

This comment has been minimized.

Copy link
Contributor Author

louismrose commented Jul 9, 2019

I'm pretty sure that the Mac build failure is a false positive, and that my changes have not introduced a regression.

The error from the CI build is:

Error: No valid signing identity available to run autoUpdater specs
    at Context.<anonymous> (electron/spec-main/api-autoupdater-darwin-spec.ts:26:15)
    at processImmediate (internal/timers.js:445:21)

And this is for the autoUpdater behavior "before each" hook for "should have a valid code signing identity" part of the test suite.

It seems like perhaps this is a CI setup failure, rather than anything to do with the code I've changed (which is in system preferences, and not in auto-updating).

@zcbenz -- do you know if there's anything more I need to do to get a passing build, or for this to be merged? I looked at re-running the build on Circle CI, but I don't have permission to do so. The Electron docs suggest that I could ask a releaser to re-run the build, but sadly the link to the list of releasers ((https://github.com/orgs/electron/teams/releasers/members) is a 404 for me.

@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented Jul 9, 2019

I have re-run the test, we can merge even with CI failing since it is obviously a flaky one.

(I want to wait for more reviews before merging this, as it is a new feature.)

@louismrose

This comment has been minimized.

Copy link
Contributor Author

louismrose commented Jul 9, 2019

Great, thanks for your help @zcbenz!

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Jul 9, 2019

The macOS codesign failure is due to the PR coming from a fork and is a known issue, if every other tests passes it's fine 👍

Copy link
Member

codebytere left a comment

lgtm now that the NSString class guard has been added

@louismrose

This comment has been minimized.

Copy link
Contributor Author

louismrose commented Jul 11, 2019

Thanks for the insightful feedback @nornagon. I've addressed the issue that you spotted.

CI is passing again now, except for the code-signing test which we can ignore. Thanks @MarshallOfSound for the tip about that one!

@nornagon nornagon merged commit 79114ff into electron:master Jul 11, 2019
7 of 9 checks passed
7 of 9 checks passed
build-mac Workflow: build-mac
Details
Artifact Comparison Changes Detected
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jul 11, 2019

Release Notes Persisted

Exposed the value of NSNotification.object to subscribers of notifications in systemPreferences.

@welcome

This comment has been minimized.

Copy link

welcome bot commented Jul 11, 2019

Congrats on merging your first pull request! 🎉🎉🎉

@louismrose

This comment has been minimized.

Copy link
Contributor Author

louismrose commented Jul 11, 2019

Thanks very much for your help, everyone!

Dumb question -- how will I know when this change makes it into a release?

@nornagon

This comment has been minimized.

Copy link
Member

nornagon commented Jul 11, 2019

You can watch release notes here: https://electronjs.org/releases

Current master will eventually become Electron 7, so the first stable version that will have this is 7.0.0.

@louismrose

This comment has been minimized.

Copy link
Contributor Author

louismrose commented Jul 16, 2019

Perfect -- thanks for the tip!

Is there anything I could do to have this considered for a backport into Electron 6 (or even earlier)? This feature is a blocker for one of my employer's big summer projects, and we'd be happy to dedicate some time to a backport if appropriate.

@nornagon

This comment has been minimized.

Copy link
Member

nornagon commented Jul 16, 2019

Unfortunately it's past the feature freeze time for Electron 6, so no new features will be accepted for backport. Once 6 is out though you'll be able to use the 7 beta series. See https://electronjs.org/docs/tutorial/electron-timelines.

@louismrose

This comment has been minimized.

Copy link
Contributor Author

louismrose commented Jul 16, 2019

Ok, great -- we can likely make a beta series work for us. Thanks @nornagon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.