flyin1501 · 2022-08-07 14:25
nv-elisp · 2022-08-07 14:42
Looks like you're delegating by detecting which notification executable is present on the user's system. This should be done once when the library is loaded and stored in a variable. That way the check does not need to be repeated at every call.
I would consider moving your magit specific code into its own optional feature which you can provide in a separate file.
Consider adding a few user customization options.
The use of a minor mode for the start notification seems like overkill to me. Nothing technically wrong with it, but after-init-hook only runs once. So no need to wrap the hook setting in a minor mode.
organic-bookworm · 2022-08-07 15:06
[(executable-find "powershell.exe")](https://github.com/kisaragi-hiu/send-notification/blob/197afad36cd08ff0a28c2439ed644b34db92a849/send-notification.el#L76-L120)
Apparently, there's [`w32-notification-notify`](https://www.gnu.org/software/emacs/manual/html_node/elisp/Desktop-Notifications.html#index-w32_002dnotification_002dnotify) available for native Windows builds.
EDIT: Reference source code for context.
lafrenierejm · 2022-08-07 23:02
> Looks like you're delegating by detecting which notification executable is present on the user's system. This should be done once when the library is loaded and stored in a variable. That way the check does not need to be repeated at every call.
I just raised https://github.com/kisaragi-hiu/send-notification/pull/1 with an alternate approach. If you (u/nv-elisp) or anyone else has feedback on the PR, feel free to post comments here or directly on the PR!
github-alphapapa · 2022-08-08 17:00
Have you considered trying to improve the built-in `notifications` library to work on more platforms?
flyin1501 · 2022-08-09 08:30
I haven't (yet) assigned my copyright, so I'm somewhat wary of touching internal code.
flyin1501 · 2022-08-09 09:20
That function uses a system tray icon plus a bubble to implement notifications, and isn't appropriate for Windows 10 or 11.
I should still use it if Windows "toast notifications" aren't available though, thank you!
flyin1501 · 2022-08-09 09:29
Thank you!
However, I personally dislike the macro based approach (Helpful's Source Code section becomes useless for a function defined like that, the byte compiler complains about unused lexical variables because their usages aren't in the generated definition), so I've opted to rewrite it with individual handler functions and a user option that gets set on load.
Sorry, that means I closed your PR, but I've put the rewritten version [in another PR](https://github.com/kisaragi-hiu/send-notification/pull/2). I'd like to think what you think about it!
organic-bookworm · 2022-08-09 12:20
Got it, thanks.