mikaelbr / node-notifier

A Node.js module for sending notifications on native Mac, Windows and Linux (or Growl as fallback)
MIT License
5.73k stars 322 forks source link

fix: fixes mapping on snoretoast activate event, fixes #291 #347

Closed ssredna closed 3 years ago

ssredna commented 3 years ago

The mapper-function for the resultantData did not map "activate" to "click", so fixed the mapper to handle this.

This introduces a breaking change.

mikaelbr commented 3 years ago

Brilliant. This should fix #291 finally. Good work.

Unsure what to do with this as a version update though. Think this might be a major version bump, even though it technically was a bug as the API has changed.

DinerIsmail commented 3 years ago

Looking forward to this fix! It sounds like a major version update to me @mikaelbr since it has a breaking change

tony19 commented 3 years ago

This PR doesn't actually fix #291 in Windows, and it appears to have no observable effect at all.

Here's the sequence of events:

  1. For some reason, the pipe used to communicate with snoretoast-x64.exe (v0.7.0) gets no data when the Windows notification is clicked (i.e., the EXE outputs no text for the click).
  2. In the pipe callback, resultBuffer (which holds the pipe's data) is set to an empty string.
  3. In snoreToastResultParser(), resultBuffer is parsed into an empty object, which is passed to a callback -- specifically utils.actionJackerDecorator().
  4. In utils.actionJackerDecorator(), the empty object argument causes the activation type to be undefined, which is passed to the userland notification callback. Normally, the activation type would be 'activate' in this case.

A simple workaround is to default the activation type to 'activate' in utils.actionJackerDecorator():

resultantData = resultantData.activationType || 'activate'; // lib/utils.js#L264
DuBistKomisch commented 3 years ago

I'm seeing the same thing as @tony19: this doesn't do anything, at least on Windows 10 20H2. The pipe never gets written to upon activation of the notification, which I believe is a bug in snoretoast which I've fixed here: https://github.com/viviedu/snoretoast/commit/c5faeceaf36f4b9fb27e5269990b716a25ecbe43

With this change, everything suddenly starts working, both plain activate events and actions passed as buttons.

I'm not sure how it's working at all for some people?

Araxeus commented 3 years ago

@DuBistKomisch Hey this looks great!

could you publish a PR with your changes to their gitlab ? It would be awesome if snoretoast would get updated, and then node-notifier too :)

Or alternatively, you could compile your changes, and make a PR here with the new binaries (this might be better since it seems that SnoreToast is very rarely updated)

as for your question > I'm not sure how it's working at all for some people? toaster with actions seems to work for me only if `appID` isn't specified (and then notifications show SnoreToast text+icon) if appID is specified, every action returns `undefined`. see #332
DuBistKomisch commented 3 years ago

@Araxeus I've got a fork of node-notifier with bumped snoretoast binaries here if that's helpful to you: https://github.com/viviedu/node-notifier/commit/4d7e6ae3b8430f544245a7c366295193b264ba33, but it also includes this other hopefully harmless snoretoast change: https://github.com/viviedu/snoretoast/commit/b1858048c37392338831f392f1bd8e531cc9a018.

I had another look at the snoretoast code and there seems to be another code path for a "background callback" which writes to the pipe properly. However it depends on writing the callback to the Windows registry when snoretoast "installs" a shortcut, so this wouldn't work for a normal shortcut created by a normal Windows installer with just the userAppModelID set (such as for our app).

This highlights another bug in node-notifier I noticed previously: there's no way to pass the 3 separate values to snoretoast's install command line flag, so you can't install its special registry callback even if you wanted to use it that way. Now that I read #332 again, NunoCuradoFuze seems to be instructing you to register the callback yourself somehow, but I'm still not sure if that will work.

Anyway, I can put up some PRs on Monday, try my fork in the meantime :)

Araxeus commented 3 years ago

@DuBistKomisch Thanks alot!! I have tested your fork and it fixes all problems with appID ! 😁 🎉

shame the we can't yet just use options.customPath to use the updated binary with the official release.. see #373

DuBistKomisch commented 3 years ago

I've submitted a PR to snoretoast at https://github.com/KDE/snoretoast/pull/15, and rebuilt it for node-notifier at https://github.com/mikaelbr/node-notifier/pull/375. Turns out this doesn't really have much to do with this PR or #291 after all since it only happens with appID, so let's have any further discussion over in the PR(s) instead.