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 activation not writing to pipe #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DuBistKomisch
Copy link

I noticed this while using node-notifier, which uses the pipe functionality to receive the response. This worked for dismissing and timeout, and for activation via the background callback. However, in the case where the app shortcut isn't installed via snoretoast, there is no background callback registered so we need to rely on the normal activation event handler, which never wrote to the pipe.

I've taken a shot at fixing this by writing the action to the pipe, including the button for click events. I didn't test text entry, it may need another extra value too.

@Araxeus
Copy link

Araxeus commented May 10, 2021

This repository is a mirror of a KDE repository. This means that developers are not looking at pull requests created in GitHub, so they probably wont see this pull request
Please see community.kde.org/Infrastructure/Github_Mirror for details on how to contribute to this and other KDE projects.

Mirror of https://invent.kde.org/libraries/snoretoast

@TheOneRing
Copy link
Contributor

I thx for reporting.
The response is indeed not handled correctly.
However I'm not sure the fix is correc.
As I haven't touched the code in a way to long time, I'll first need some time to read my way back into it 😅

@DuBistKomisch
Copy link
Author

No worries, let me know if there's anything I can fix up, and if you'd rather me reopen this on the KDE gitlab instead.

@rlaace423
Copy link

Waiting for merging :)

Could I get detail information for snoretoast build? I tried cmake it, and open with latest visual studio but didn't work.

@DuBistKomisch
Copy link
Author

cmake -A <Win32/x64> -B <build32/build64>
cmake --build <build32/build64> --config <Debug/Release>

(pick one or the other between angle brackets)

This is what's been working for me, with just Visual Studio Build Tools 2019 with the C++ workload.

@rlaace423
Copy link

cmake -A <Win32/x64> -B <build32/build64>
cmake --build <build32/build64> --config <Debug/Release>

(pick one or the other between angle brackets)

This is what's been working for me, with just Visual Studio Build Tools 2019 with the C++ workload.

Thank you! I'll try.

@TheOneRing
Copy link
Contributor

I haven't forgotten it... just work.. life and stuff 😅

@smadep
Copy link

smadep commented Sep 17, 2021

what's the status of this? I'm waiting for this also in context with issue in node-notifier

@TheOneRing
Copy link
Contributor

I finally had a look, but how to reproduce the initial issue?

If I call snoretoast like this:

SnoreToast.exe -t test -m message -pipename \\.\pipe\foo -id 0 -b Hi;There
action=buttonClicked;notificationId=0;pipe=\\.\pipe\foo;button=Hi;version=0.8.0;
Action: buttonClicked 4


SnoreToast.exe Notification: 1 exited with: 4

Without any I get no buttons at all for the first notification (snoretoast registers its self), the second notification hast the response from above.

If I provide an id that is not installed, I never get any button to click.
image

@DuBistKomisch
Copy link
Author

From memory, you must pass a valid app ID, otherwise Windows refuses to include any controls in the notification at all, for whatever reason.

@TheOneRing
Copy link
Contributor

Ok could you fill in some steps to reproduce?
It only happens when activated from the action center after it initially timed out?

@Araxeus
Copy link

Araxeus commented Sep 21, 2021

Without an appID it works ok, SnoreToast register itself and the response from the pipe on button clicks works as intended

But if you specify a valid appID - the response from button clicks (=which one was clicked) is missing

it appears that data is not being returned over the pipe. None of the pipe's events actually fire when an appID is specified and a button is clicked. SnoreToast does however know the a button has been clicked and the exit code is correct.

Anyways this PR fixes the problem for me

@DuBistKomisch
Copy link
Author

  1. Call SnoreToast with the -appID, -pipeName and optionally -b flags.
  2. Click the notification, or the button in the notification.
  3. No activate or button message is received over the pipe.

I know a real AppUserModelID is required for buttons to show up, but I think you might be able to try with the activate case anyway, as long as you pass some string to -appID to reach this code path.

This is basically what is happening in the original downstream issue I ran into mikaelbr/node-notifier#332, where the node library passes those args on the command line and (should) emit pipe messages back to JavaScript.

@abdulghanitech
Copy link

Any update guys? This fix will help a lot

@Araxeus
Copy link

Araxeus commented Jan 1, 2022

@TheOneRing Happy new year

Could you give us a present for the new year? merge this fix and release a new version? 🥺

Its been almost a year since this PR was created..

@TheOneRing
Copy link
Contributor

TheOneRing commented Jul 22, 2022

Hm I gave it another look and the main problem now is that if the app is correctly registered we will receive two callbacks.
Once from the systems callback and once from the write here.
So ideally there would be a way to detect whether we are properly installed and then conditionally directly notify using the socket.

For the detection: the first time we run we could use

tLog << "Failed to retreive NotificationSettings ensure your appId is registered";
but the calls succeeds the second time its called...
In order to detect whether an app id is registered I tried IApplicationActivationManager->ActivateApplication but it appears that only works with applications installed from the store.

Btw, with this patch it appears that we can only receive a clicked as the buttons are not displayed if the app is not registered.

As snoretoast can be used to install the shortcut, I'd rather suggest that the caller of the app calls -install on startup.
This will then allow the use of notification buttons and text boxes.

PS: to reproduce the issue I used the python example and removed the install step

run(["snoretoast", "-install", "Snoretoast Python Example", sys.executable, APP_ID])

@TheOneRing
Copy link
Contributor

The issue is probably fixed on master.
I'll give it some testing with a store app and if things work out there will be a release.

@TheOneRing
Copy link
Contributor

@Araxeus
Copy link

Araxeus commented Jul 23, 2022

from my small testing using node-notifier - the issue is not fixed in the binaries you posted, while compiling using this patch does fix the issue

@TheOneRing
Copy link
Contributor

In that case please modify the python example in to a minimum reproducible.

@Araxeus
Copy link

Araxeus commented Dec 14, 2022

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.

7 participants