toast notifications(+) #17

2024-12-20T18:33:33-05:00

mshafer1 commented

2024-12-20 18:33:33 -05:00

(Migrated from github.com)

Why

I'm looking for a Windows client that can run as a service (or minimized to notifactions tray) -> not just minimized to the task bar so that it runs "in the background" AND that has an option to make the notifications stay up until dismissed (because I'm pushing alerts that are important and need to be seen).

ntfysh-windows already has the first, but by using the default Windows notification tooling, the ability to set the notification to stay for requested time is hampered.

This PR replaces calling the built-in Windows Toast notification for using an animated window. This provides the flexibility to have a timeout progress bar or have an infinite/until-dismissed notification.

What I have done

Make an option to use "custom" notifications (if False - the default - still uses Windows toad notification, if true, use new notification).

That custom WinForm still:

shows title, icon, and message
now offers a scroll bar if the message is very long

and now:

uses a progress bar to show a timeout for how long the message will display (can be disabled in settings)
cancels that timeout if the dialogue is clicked on
allows copy/pasting the title and message
allows dismissing by click the "x" button
if timeout is set to 0 in settings, stays open until dismissed or another message is sent.

I also introduced ntfysh_client.Themes namespace which contains a default (based on the current Settings dialogue and a darkMode to allow the user to choose the style/theming of their pop ups.

Testing / Demo

Demo: https://youtu.be/BF3kzUK8DR0

# Why I'm looking for a Windows client that can run as a service (or minimized to notifactions tray) -> not just minimized to the task bar so that it runs "in the background" AND that has an option to make the notifications stay up until dismissed (because I'm pushing alerts that are important and need to be seen). ntfysh-windows already has the first, but by using the default Windows notification tooling, the ability to set the notification to stay for requested time is hampered. This PR replaces calling the built-in Windows Toast notification for using an animated window. This provides the flexibility to have a timeout progress bar or have an infinite/until-dismissed notification. # What I have done Make an option to use "custom" notifications (if False - the default - still uses Windows toad notification, if true, use new notification). That custom WinForm still: - shows title, icon, and message - now offers a scroll bar if the message is very long and now: - uses a progress bar to show a timeout for how long the message will display (can be disabled in settings) - cancels that timeout if the dialogue is clicked on - allows copy/pasting the title and message - allows dismissing by click the "x" button - if timeout is set to 0 in settings, stays open until dismissed or another message is sent. I also introduced ntfysh_client.Themes namespace which contains a `default` (based on the current Settings dialogue and a `darkMode` to allow the user to choose the style/theming of their pop ups. # Testing / Demo Demo: https://youtu.be/BF3kzUK8DR0

alexhorner commented

2024-12-20 19:04:32 -05:00

(Migrated from github.com)

Hey there @mshafer1,

I haven't been able to do a full code review yet, but will do so tomorrow when I get the chance.

I think my main response right now would be your use case doesn't fit everyone's requirements. I for one wouldn't like the new notifications and would find them extremely frustrating and distracting, but I can absolutely see the appeal in the more obstructive and attention grabbing style.

I think a full replacement is a little too far, but a good compromise would be to add a configuration option which allows the user to choose which type of notification they would like to receive, with the native Windows one being the default as this is what users so far have come to expect from the application.

Hey there @mshafer1, I haven't been able to do a full code review yet, but will do so tomorrow when I get the chance. I think my main response right now would be your use case doesn't fit everyone's requirements. I for one wouldn't like the new notifications and would find them extremely frustrating and distracting, but I can absolutely see the appeal in the more obstructive and attention grabbing style. I think a full replacement is a little too far, but a good compromise would be to add a configuration option which allows the user to choose which type of notification they would like to receive, with the native Windows one being the default as this is what users so far have come to expect from the application.

mshafer1 commented

2024-12-20 19:20:50 -05:00

(Migrated from github.com)

I think a config option is a good idea. I contemplated adding that in round
one but thought but wasn't sure if it should be included in the MVP.

I could also see a strong case for making them sized more like before, but
beyond just shrinking it, I would argue non MVP.

I also don't actually want the status/progress bar, as my use-case is
important only (i.e. would be set to not timeout).

Would you rather I take that part back out, or make that a config option as
well.

On Fri, Dec 20, 2024, 18:04 Alexander Horner @.***>
wrote:

Hey there @mshafer1 https://github.com/mshafer1,

I haven't been able to do a full code review yet, but will do so tomorrow
when I get the chance.

I think my main response right now would be your use case doesn't fit
everyone's requirements. I for one wouldn't like the new notifications and
would find them extremely frustrating and distracting, but I can absolutely
see the appeal in the more obstructive and attention grabbing style.

I think a full replacement is a little too far, but a good compromise
would be to add a configuration option which allows the user to choose
which type of notification they would like to receive, with the native
Windows one being the default as this is what users so far have come to
expect from the application.

—
Reply to this email directly, view it on GitHub
https://github.com/lucas-bortoli/ntfysh-windows/pull/17#issuecomment-2557898786,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AATSJ4M2YEENZOZPDQPYPFT2GSWCPAVCNFSM6AAAAABUAAD4F2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNJXHA4TQNZYGY
.
You are receiving this because you were mentioned.Message ID:
@.***>

I think a config option is a good idea. I contemplated adding that in round one but thought but wasn't sure if it should be included in the MVP. I could also see a strong case for making them sized more like before, but beyond just shrinking it, I would argue non MVP. I also don't actually want the status/progress bar, as my use-case is important only (i.e. would be set to not timeout). Would you rather I take that part back out, or make that a config option as well. On Fri, Dec 20, 2024, 18:04 Alexander Horner ***@***.***> wrote: > Hey there @mshafer1 <https://github.com/mshafer1>, > > I haven't been able to do a full code review yet, but will do so tomorrow > when I get the chance. > > I think my main response right now would be your use case doesn't fit > everyone's requirements. I for one wouldn't like the new notifications and > would find them extremely frustrating and distracting, but I can absolutely > see the appeal in the more obstructive and attention grabbing style. > > I think a full replacement is a little too far, but a good compromise > would be to add a configuration option which allows the user to choose > which type of notification they would like to receive, with the native > Windows one being the default as this is what users so far have come to > expect from the application. > > — > Reply to this email directly, view it on GitHub > <https://github.com/lucas-bortoli/ntfysh-windows/pull/17#issuecomment-2557898786>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AATSJ4M2YEENZOZPDQPYPFT2GSWCPAVCNFSM6AAAAABUAAD4F2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNJXHA4TQNZYGY> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

alexhorner commented

2024-12-20 19:25:07 -05:00

(Migrated from github.com)

A feature MVP should strive to at least not break existing expectations or functionality for sure, so I think it's an important addition for a config option to choose.

You might wish to add an option to show or hide the counter and/or progress bar, but I think it's a good attention catching feature once again and has value.

The size and style could do with being made more consistent with the rest of the application, but for now I would say it works fine

A feature MVP should strive to at least not break existing expectations or functionality for sure, so I think it's an important addition for a config option to choose. You might wish to add an option to show or hide the counter and/or progress bar, but I think it's a good attention catching feature once again and has value. The size and style could do with being made more consistent with the rest of the application, but for now I would say it works fine

mshafer1 commented

2024-12-20 22:49:52 -05:00

(Migrated from github.com)

@alexhorner, all right, I got a draft of the settings dialogue updated and have tested that everything but the dark-mode switch works (that's not implemented yet). I'll plan to work on making the default be winForms default colors in the morning.

mshafer1 (Migrated from github.com) reviewed 2024-12-20 22:53:52 -05:00

ntfysh_client/MainForm.cs

mshafer1 (Migrated from github.com) commented

2024-12-20 22:53:43 -05:00

I noticed this while debugging.... while I didn't do a proper Chesterton's Fence investigation on it, it seem pretty wrong...., I'll dig into history on this more later.

I noticed this while debugging.... while I didn't do a proper [Chesterton's Fence investigation](https://en.wikipedia.org/wiki/G._K._Chesterton#Chesterton's_fence) on it, it seem pretty wrong...., I'll dig into history on this more later.

alexhorner (Migrated from github.com) reviewed 2024-12-21 03:50:34 -05:00

ntfysh_client/MainForm.cs

alexhorner (Migrated from github.com) commented

2024-12-21 03:50:34 -05:00

Thaaaat looks like a keyboard ninja tab tab enter move typo 🤣 Was that me? Whoops!

alexhorner (Migrated from github.com) requested changes 2024-12-21 06:19:34 -05:00

alexhorner (Migrated from github.com) left a comment

HI @mshafer1

Please see attached review comments for my full code review.

Thanks for your work!

HI @mshafer1 Please see attached review comments for my full code review. Thanks for your work!

ntfysh_client/MainForm.cs

alexhorner (Migrated from github.com) commented

2024-12-21 05:52:28 -05:00

In keeping with established convention please use a _ and remote the this.

ntfysh_client/MainForm.cs

alexhorner (Migrated from github.com) commented

2024-12-21 06:13:13 -05:00

Nitpick: newline

this.notificationDialog.ShowNotification
(
    ...
);

Nitpick: newline ```csharp this.notificationDialog.ShowNotification ( ... ); ```

alexhorner (Migrated from github.com) commented

2024-12-21 06:15:25 -05:00

This seems a little cursed. If that's the best way to do it, fair enough, but if there's a better way (such as changing the type of timeout_ms) that might be better because we're throwing away precision.

This seems a little cursed. If that's the best way to do it, fair enough, but if there's a better way (such as changing the type of `timeout_ms`) that might be better because we're throwing away precision.

ntfysh_client/MainForm.cs

alexhorner (Migrated from github.com) commented

2024-12-21 05:54:01 -05:00

Can UseNativeWindowsNotifications and UseCustomTrayNotifications not be merged?

Can `UseNativeWindowsNotifications` and `UseCustomTrayNotifications` not be merged?

ntfysh_client/MainForm.cs

alexhorner (Migrated from github.com) commented

2024-12-21 05:55:28 -05:00

A blank line between the previous if closure and this one's opening, and a comment consistent with the one above the previous if would be good

@@ -0,0 +41,4 @@
             {
                 // close the current notification
                 HandleTimeout(null, null);
             }

@@ -0,0 +58,4 @@
             {
                 _displayTimeoutTimer.Stop();
                 _displayTimeoutTimer.Dispose();
             }

@@ -0,0 +64,4 @@
             {
                 _updateTimer.Stop();
                 _updateTimer.Dispose();
             }

@@ -0,0 +148,4 @@
             {
                 BringToFront();
                 AnimateWindow(

@@ -0,0 +153,4 @@
                     time: 250,
                     flags: NFWinUserAnimateWindowConstants.AW_SLIDE | NFWinUserAnimateWindowConstants.AW_VER_NEGATIVE
                 );
             }

@@ -0,0 +170,4 @@
         private void UIThreadAnimatedHideWindow(object? sender, EventArgs? e)
         {
             AnimateWindow(

@@ -0,0 +174,4 @@
                 Handle,
                 time: 250,
                 flags: NFWinUserAnimateWindowConstants.AW_SLIDE | NFWinUserAnimateWindowConstants.AW_VER_POSITIVE | NFWinUserAnimateWindowConstants.AW_HIDE
             );

@@ -0,0 +269,4 @@
             {
                 _shownStopwatch.Stop();
                 _shownStopwatch = null;
             }

@@ -0,0 +285,4 @@
                 SoundPlayer loadedSound = new(defaultSoundPath);
                 loadedSound.Play();
             }

@@ -26,1 +69,4 @@
         }
         #endregion
         public SettingsDialog()

@@ -0,0 +98,4 @@
                 ProgressBar1.Visible = false;
                 LblTimeout.Visible = false;
             }