royceschultz / ComfyUI-Notifications

ComfyUI nodes for sending notifications when a workflow completes
8 stars 2 forks source link

[FEATURE REQUEST] Unified notification node #1

Closed alessandroperilli closed 4 months ago

alessandroperilli commented 4 months ago

Your System Notification node is very welcome and I plan to add it to the upcoming AP Workflow 10 for ComfyUI. Thank you for sharing it with the community.

May I suggest that instead of having two distinct nodes, you have a single one with two toggles?

Toggle 1 enables sound. Toggle 2 enables browser notification.

IMO it would be more compact and versatile.

Related to this, I have a couple of extra suggestions:

  1. Allow users to customize the message seen in the browser notification.
  2. Put some guidance in the Readme about how to enable notifications for non-HTTPS deployments of ComfyUI. That would probably avoid a lot of frustration for people.
royceschultz commented 4 months ago

Thanks for you feedback and suggestions!

On a unified node, given they have unique sets of arguments, I think these make sense as separate nodes. At least personally, I don't use them in conjunction, it's more of a one or the other type thing.

I like the idea of a customizable message. I will add this feature soon. As related to the unified node, this feature would make the arguments even more distinct between sound and system notif.

Could you clarify any issues you had with setting up notification permissions? At least on my system (macos + firefox/chrome), I get a browser popup asking for notification permission if it's not granted. IIRC in firefox the popup was 'silent' when opening an existing workflow rather than adding the node manually. Instead of a popup, the notification icon in the search bar would wiggle - easy to miss if you don't know what to look for. Because of this I added notification permission checks each time it's called so I think it should ask for permission the first time it's run, but failing to notify the first time. Just my recollection of development, I would need to review again to verify.

As related to the unified node, play sound has no such permission issue. I suppose as a backup it could be useful. I'm thinking a 3rd node as a 'unified notifier' might be useful for this case.

royceschultz commented 4 months ago

Please pull the latest commits to give it a try. Let me know if you notice any issues on your system.

Pending feedback: Notification permissions streamlining.

royceschultz commented 4 months ago

btw your selected works look fantastic! What techniques did you use to get such high resolution and influence the styling? I suppose I'll have to checkout your AP workflow.

What TTS model did you use for fake show?

alessandroperilli commented 4 months ago

Thank you for addressing the request You released faster than I could provide feedback!

The new unification node looks good. I think having both individual nodes and a unified one with a compact design adds a lot of flexibility.

Not something I personally need right now but, perhaps in the future, you could add a 3rd option for email notifications. It's a significantly more complicated setup, but I'm amazed that nobody ever developed a node for that.

I have a question about this repo: I saw that you contributed back to the pythongossssss repo the System Notification node. Do you plan to contribute back also the new unified node?

APW already requires the installation of that repo and, of course, the fewer custom node suites for my users to install, the better.


Re the challenges with enabling browser notifications

My setup is mildly more complicated than the average. I run ComfyUI on a local machine and connect to it from another (with macOS).

In that setup, if you connect to ComfyUI with HTTP, the browser considers the destination insecure and it refuses to allow notifications. The javascript generates a popup that says something to the effect of : Your browser blocks all notifications from this website. Change it in the browser settings.

At that point, even if create an exception in the browser settings, it still blocks by default all notifications from the remote ComfyUI server in the local network.

This happens with Vivaldi (a flavor of Chrome) and with Safari. I spent 4h yesterday on this problem and tried various experimental flags to bypass the problem, without success.

In the end, this situation pushed me to do something I planned to do anyway for quite some time: run ComfyUI on SSL with a self-signed certificate. I fully documented the procedure on the AP Workflow website.

As soon as I connected to ComfyUI with HTTPS, the browser notification behavior changed to the usual question: Do you want to enable notifications for this website? You allow it and life is good.

I understand this is an edge case, and maybe you don't want to create complex documentation about it, but it's worth keeping it in mind in case somebody opens an issue about it.


Re my AI works

Thank you. Every work is done with APW, using a multitude of functions which vary depending on each image: Prompt Enhancer, IPAdapter (sometimes, two in a chain), Refiner, Face Detailer and Hand Detailer, Upscaler (SUPIR) or Upscaler (CCSR).

Fake Show synthetic voices are generated by Tortoise TTS, which still today remains state of the art TTS, no matter what people say. If you have the patience to sit through the entire 3min video, you'll have a sense of the range of emotions I managed to express 1.5 years ago with that engine.

I talked more about this in one of the Discord channels on the Banodoco server.

That's it. Thanks again for creating the unified notification node and for adding the capability to change the message!

royceschultz commented 4 months ago

Re: Email Notifications

Email notifications are non-trivial to setup given the steps in user authentication and differences in email providers, but I like the idea as a way for a headless server to notify the user. As a step towards this goal, I added a webhook node which achieves the headless behavior and allows users to integrate easily with either their own webserver for customized actions or 3rd party webhooks such as Twillio (SMS), Slack, or Home Assistant (none of these tested in development of the webhook node).

Re: Merging with Custom-Scripts

I don't plan to actively maintain the differences between Custom-scripts and this repo. Perhaps rarely I will submit a PR to update important changes. I preferred a simple, bare-bones solution to the notification problem over the all-in-one solution of custom-scripts that included some features I didn't want.

Re: Notifications Permissions Issues

I'm running a similar setup - ComfyUI on a local linux box, then connecting on my Mac laptop, specifically over ssh port forwarding, but still using http in the browser. I observe no issue with permissions in Firefox, Chrome, or Safari. In each I get a popup asking for permission and the notifications appear as expected. Perhaps this is a security/privacy feature of Vivaldi?

alessandroperilli commented 4 months ago

A webhook is a flexible enough approach. One could use it with Zapier and leverage whatever system they want. Thanks!

To test it, I created a Discord webhook. I had to modify the JSON format string, but it worked pretty well. I wish there would be a way to send the image over the webhook without having to host it somewhere public.

The notification permission issue I had was also present on Safari. I have no idea why I didn't experience what you experienced (and what should happen). I'll go back to the problem when I have more time.

alessandroperilli commented 4 months ago

By the way, your nodes are now featured in this sneak peek video: https://www.youtube.com/watch?v=0q39PjDmfoY

Thanks again!

royceschultz commented 2 months ago

Regarding the notification issue, notification support is only allowed in secure contexts so HTTPS is required to enable system notifications. It seems localhost is an exception. I was using localhost via port-forwarding to my laptop. It's too bad Firefox doesn't have a setting to change this behavior.