Details
- Reviewers
kamil tomek - Commits
- rCOMM723f24eceb46: [desktop] Handle windows notifications
Check if the app is correctly installed. Run the app, connect to the keyserver and check if it correctly gets the notifications. Check if it redirects to the app after clicking on the notification.
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| desktop/src/main.js | ||
|---|---|---|
| 258–260 ↗ | (On Diff #24571) | This is needed because without this the app name in the notifications was displayed as electron.app.comm. |
| desktop/src/push-notifications.js | ||
| 15–17 ↗ | (On Diff #24571) | We load the module conditionally because to load it correctly, the windows runtime installer has to be run on the user's machine. This was added in D7253. In short, Comm is run two times: once during installation so we can do any needed setup (like creating shortcuts and running the windows runtime installer), and after the installation is complete, it's run normally. In the first case (before we run the runtime installer) the isNormalStartup call will return false and we will skip loading this module. |
| 27–29 ↗ | (On Diff #24571) | We have to convert the object returned by the native module to a string. Unfortunately, it doesn't implement the iterator methods so we have to use this loop. |
| 69 ↗ | (On Diff #24571) | This is going to be replaced by a Object ID created in ENG-3562 before landing. |
| 101 ↗ | (On Diff #24571) | Without this the icon for the windows notifs isn't displayed. |
| desktop/flow-typed/windows-pushnotifications_vx.x.x.js | ||
|---|---|---|
| 1–5 ↗ | (On Diff #24571) | I'm going to create a task to fill this in. |
| desktop/src/main.js | ||
|---|---|---|
| 258 ↗ | (On Diff #24571) | Is this value the same for all the Windows version, e.g. on 64bit? |
| 258–260 ↗ | (On Diff #24571) | Can we set this also on iOS? |
| desktop/src/push-notifications.js | ||
| 27–29 ↗ | (On Diff #24571) | What is the type of event.payload? Does this conversion method correctly handle e.g. emojis? |
Correctly handle all strings. Tested by sending emojis and non-ascii letters.
| desktop/src/main.js | ||
|---|---|---|
| 258 ↗ | (On Diff #24571) | According to the docs there are no other windows values. |
| 258–260 ↗ | (On Diff #24571) | This is a Windows-only api |
| desktop/src/push-notifications.js | ||
| 27–29 ↗ | (On Diff #24571) | event.payload is a wrapper around a Windows API type so it's not a standard node type. The underlying type is some sort of byte array but I couldn't figure it out exactly. |
| desktop/src/push-notifications.js | ||
|---|---|---|
| 24 ↗ | (On Diff #24695) | https://nodejs.org/api/buffer.html#static-method-bufferfromstring-encoding it seems like encoding should provided when the first argument is a string. Is that the case? If it is a string, why do we convert it again to string? I guess the intention was rather to convert an array of bytes into a string, assuming that the array is encoded using utf8. In that case the encoding should be set for toString method. https://nodejs.org/api/buffer.html#buftostringencoding-start-end By the way, are we sure that endianess is the same on both ends? |
Updated the payload conversion to string. I've finally figured out the event.payload object. It's an array wrapper created by NodeRT. It has only length property and {[number]: number} index accessor (but it's not an array).
It was somehow working with just Buffer.from(event.payload) but it was unsafe, and according to the node documentation it shouldn't work. Creating a new array will be a bit slower, but it's safer and for now we are sending only small notifications. I'm going to create a followup task with a way of optimizing it (adding a method that adds a conversion to string on the c++ side).