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

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

Merged
merged 1 commit into from
Oct 28, 2020
Merged

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

merged 1 commit into from
Oct 28, 2020

Conversation

ssredna
Copy link
Contributor

@ssredna ssredna commented Oct 28, 2020

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 mikaelbr linked an issue Oct 28, 2020 that may be closed by this pull request
@mikaelbr
Copy link
Owner

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.

@mikaelbr mikaelbr merged commit e55bd8f into mikaelbr:master Oct 28, 2020
@DinerIsmail
Copy link

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

Gvozd added a commit to Turbo87/webpack-notifier that referenced this pull request Dec 7, 2020
@tony19
Copy link

tony19 commented Jan 22, 2021

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
Copy link

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: viviedu/snoretoast@c5faece

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
Copy link
Contributor

Araxeus commented May 7, 2021

@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
Copy link

DuBistKomisch commented May 8, 2021

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

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
Copy link
Contributor

Araxeus commented May 8, 2021

@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
Copy link

I've submitted a PR to snoretoast at KDE/snoretoast#15, and rebuilt it for node-notifier at #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.

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.

Click event doesn't work
6 participants