zulip-desktop
zulip-desktop copied to clipboard
zulip
[WIP] Inline reply for Windows notifications
What's this PR do? This is a work-in-progress. Closes #392 Any background context you want to provide?
Screenshots?
You have tested this PR on:
- [ ] Windows
- [ ] Linux/Ubuntu
- [ ] macOS
This is starting work to first display WinRT notifications using the electron-windows-notifications package. This is not at all fit for review.
At the moment I am not able to get a toast notification or be able to get the code to run (I tried by using logger and seeing the log files). VS Code gives the following message: Could not find a declaration file for module 'electron-windows-notifications', even though I have successfully installed the package with npm.
I have based winrt-notifications.js on darwin-notifications.js. From what I understand, helpers.js is platform-independent module which I can get data from to display notification and as well as for inline reply.
Despite many attempts, I have not been able to install electron-windows-interactive-notifications on my system, so I have not added that part.
Any help is much appreciated. I will try to complete this feature in incremental commits.
@Swapnilr1 thanks for working on this! I was struggling with similar options for this particular npm module, and raised an issue after digging around. Check out this issue (a helpful place to start would be checking your Visual Studio Tools version). I haven't gotten around to that yet.
@kanishk98 My VS versions are right, actually the problem is even after I configure
"interactive-notifications": {"protocol": "zulip://"} in package.json in the zulip-electron folder, for some reason the install scripts do not detect that. There seems to be a lack of documentation as well.
I am waiting for a response to https://stackoverflow.com/questions/54923137/how-to-install-node-win-shortcut-by-using-npm Fixing that should finally allow me to at least get toast notifications working.
Hi @Swapnilr1 I was also struggling with the same problem (could not find a declaration file for module) but i was working with an another module though i was able to use its function does this apply same for you? by the way you can fix this check fail by writing code according to rules.
@rrhythmsharma Yes, actually while doing git commit the linter ran and gave errors, but there is no point in fixing the issues found by ESLint because, at present notifications are not working, so this either way cannot be merged.
but i was working with an another module though i was able to use its function
Sorry, but I didn't really get what you meant here. You mean you were able to execute functions from the module despite the Could not find a declaration file error?
@Swapnilr1 yes, i was able to execute function from the module despite the Could not find a declaration file error, that's why asked you does this apply same for you?.
@rrhythmsharma That's interesting. Which module were you working with? In my case, it was not working, I put a log statement just before and just after the function call and only the first one executed.
@kanishk98 @rrhythmsharma OK it turns out that installing electron-windows-notifications via npm is not enough to use it in an Electron app. You need to follow the instructions here: https://github.com/electron/electron/blob/v0.37.2/docs/tutorial/using-native-node-modules.md#using-native-node-modules
Also, yes the missing type declaration was not an issue. Turns, out the error log for why I could not get any function to run was in Developer Tools for the active tab, and not in Developer Tools for the main app.
I finally got a toast notification to get displayed: except that it displays 4 notifications at once. And I need to fill the content for notifications.
Still some time to go before inline replies are supported, but I am optimistic that now that the modules are working, I should be able to finish this by March end.
OK it turns out that installing electron-windows-notifications via npm is not enough to use it in an Electron app. You need to follow the instructions here: https://github.com/electron/electron/blob/v0.37.2/docs/tutorial/using-native-node-modules.md#using-native-node-modules
I think electron-builder does handle this problem (building the native modules).
@akashnimare Yes, probably when an installable version is made by electron-builder it won't be an issue, but for the development version which is run by npm run dev I think the solution I linked to is required?
Also, is it just me or is it a known issue that the development version of Windows crashes on file changes (it tries to restart the app upon file changes but crashes?)
I want to implement this, but I am not able to find the time right now. I will be able to continue working on this after a week.
Both electron-windows-notifications and electron-windows-interactive-notifications reply on old versions of Windows SDK (and different ones), and of VS Build Tools.
@akashnimare I would really like to work on this, but I won't be able to find time till selected students for Google Summer of Code are announced because I am busy contributing to projects I am applying to.
If this is a high priority feature, I will leave it to someone else and can offer assistance in resolving issues with VS Build Tools etc.
@akashnimare Is somebody else working on this? Or can I continue working on this?
@priyank-p @akashnimare I finally got inline replies working - although I am using a work-around which leads to display of two notifications. This PR is not ready to merge yet, so I need some help to fix issues. Let me know if you need any help in installing the packages on your machines. Hope to get it merged soon.
Heads up @Swapnilr1, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.
