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

Add more control over notifications #13

Merged
merged 38 commits into from
May 11, 2022
Merged

Add more control over notifications #13

merged 38 commits into from
May 11, 2022

Conversation

sergiou87
Copy link
Member

This PR introduces more control over notifications:

  • Check whether or not the app has permission from the user to display notifications.
  • Request permission to the user to display notifications (macOS only).
  • Handle notifications from previous app sessions.

The changes require to support all that on macOS require us to invoke all the notifications code from the main process on an Electron app. That's why this PR also replaces the previous sample script with an Electron sample app, because it eases testing for us in a more similar environment to the GitHub Desktop app.

package.json Outdated
"version": "0.1.7",
"version": "0.2.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

I will revert this before merging and manage the release separately. I also want to enable CI for macOS before doing the release, of course, but in a separate PR 😳

Comment on lines +112 to +113
auto userInfoString = Utils::JSONStringify(env, userInfoObject);
userInfo = Utils::utf8ToWideChar(userInfoString);
Copy link
Member Author

@sergiou87 sergiou87 May 9, 2022

Choose a reason for hiding this comment

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

This is using JavaScript's JSON.stringify function to serialize the userInfo object, which will be later stored in the launchArgs string (yup no way of storing it in a structured way…), to then be deserialized with JavaScript's JSON.parse function before sending it back with the notification callback.

Not very happy about this, but I couldn't think of a better way other than exposing all the Windows notifications API we need to JS and do most of the work from there, but it was a ton of work and didn't want to make this PR bigger than it already is 😂 I might tackle it in a different PR.

Comment on lines +24 to +25
// Formats the launch args like this: <notificationID>;<userInfo JSON string>
std::wstring formatLaunchArgs(const std::wstring &notificationID, const std::wstring &userInfo);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is another thing I'm not super proud of, and another good reason to move logic to JS where it's easier to serialize/deserialize and have some compiler-time help for this data.

@sergiou87 sergiou87 marked this pull request as ready for review May 9, 2022 16:35
Copy link

@tidy-dev tidy-dev left a comment

Choose a reason for hiding this comment

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

✨ I appreciated you breaking things down into smaller functions - much easier to digest syntax I wasn't familiar with when I knew what it was supposed to be doing. :D

Notes on testing:

  • attempted to test via the sample-app but seems it was blocked from notifying on mac. (Discussed this in slack).
  • tested with the draft pr in Desktop and was able to still show the notification and when I disabled/enabled permissions it did as expected. :D But, I am not terribly sure how to test the requesting permission part.
  • I was thinking that if I opened desktop dev, hit show notification, closed desktop dev, opened desktop dev, clicked on notification that it might show it in the new instance. It did nothing. (But, maybe I misunderstood and that is not part of this.) :/

@sergiou87
Copy link
Member Author

I was thinking that if I opened desktop dev, hit show notification, closed desktop dev, opened desktop dev, clicked on notification that it might show it in the new instance. It did nothing. (But, maybe I misunderstood and that is not part of this.) :/

That is correct: the Help -> Show Notification is a bit special because it's created ad-hoc. Real notifications (from checks failing or PR reviews) are created from an Alive event sent from dotcom. In that case, we display the notification and attach the Alive event as the notification's userInfo.

Later, when you close the app and reopen it again, if you click the notification, Desktop will recover the Alive event from the notification's userInfo, and will process it accordingly.

The notification from Help -> Show Notification doesn't have any kind of data that we pass along with the notification and hence why it has no effect when you reopen the app. If you try an actual notification, it should work!

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

2 participants