-
Notifications
You must be signed in to change notification settings - Fork 576
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
[Notifications] [Windows] Native interactive notifications #946
Conversation
* Add support for protocol commands * Remove node-notifier dependency
I love this app! That being said, I have a couple of EPIC SUGGESTIONS:
I'm virtually certain this is in the wrong place, but am unfamiliar with github and busy with something else... so here you go. Thanks! |
Moved it to #947! |
`xml_logo_ascii` `xml_logo_icons` `xml_logo_icons_notext` `xml_hero` `xml_banner_bottom` `xml_banner_top_custom` `xml_banner_centered_bottom` `xml_banner_centered_top`
@th-ch @Zo-Bro-23 @MiepHD @Liknox or anyone who might see this What do you think of the presets?
|
I'd let them choose between
|
I've updated the post with my thoughts on each of them, but overall is there a reason not to include any of them? more customization might be better if there isn't a tradeoff I think I might just have a submenu called |
This is AWESOME! Personally, don't really like the look of the ASCII versions, but it doesn't hurt to keep them in. The others are really visually pleasing, and of course, the more options the better (particularly love the hero and big banner ones)! How are you planning to implement these options though? Only if presented with a set of images like these will the user know which one to pick. Simply showing a list of big banner, hero, etc will be hard for the user to know what each one looks like. If you figure out a way to list all the possible options (maybe have a button that brings up a pop-up showing demos for how all these options look like), then you've got yourself a perfect feature! Looking forward to seeing this implemented 🙃 |
Yeah if I made the interactive-notifications plugin from scratch today I probably wouldn't include something like that
very good idea 🌟 - will require quite a bit of additional code but might be worth it Thanks for the encouragement :D |
I've decided for now I'll do it like so: const pluginOptions = {
toastStyle: 1, // 1-7
hideButtonText: false
}
module.exports.ToastStyles = {
logo: 1, // logo_icons
hero: 2,
banner_top_custom: 3,
banner_bottom: 4,
banner_centered_bottom: 5,
banner_centered_top: 6,
legacy: 7 // logo_ascii
}
I'll look into it when I have some downtime. meanwhile you can add me on discord Araxeus#0819 |
So these are the options that will show up on the menu? If I understand this correctly, the user can manually select options like
Cool, that's great! Thanks so much 😁 |
No, the only customizable option right now is
|
Oh. Having the image previews might be good in that case. Also try if you can have these as options, which will allow for more customization (mix-and-match of different settings). |
I understand your meaning, but think about it - what can you actually mix-and-match here? Each of the presets is unique. the only thing that I can think of that could be an option is
compare that to |
You're right - my bad! The show button text is probably the only one that can be an option, and you've already incorporated that. I thought about having choices for the other properties, but that gets confusing, because you can't have BOTH hero and banner, and you can't have BOTH text above and text below. So I've got nothing more to add - presets are the best way to do this! |
This PR is actually mostly complete now with If you are on windows, you can clone the branch and try it out! 🤘🚀 my favorite ended up being in toasts with banners the picture format depends on the kind of video you are watching:
It is done this way because changing the 1x1 to a 2x1 looks pretty bad,
I'm not even sure this is needed anymore, you can just select any of them and it will automatically update without needing to restart the app which makes it easy to test each of them (tho I agree a dedicated panel for a demo would be cool, as it stands I'm not sure there is that much to gain) |
My laptop's gone in for repair, so I'm on Ubuntu right now 🥲. Looking forward to testing it out when I get my laptop back! |
const singleton = (fn) => { | ||
let called = false; | ||
return (...args) => { | ||
if (called) return; | ||
called = true; | ||
return fn(...args); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can extract this function so that it is available app wide
@@ -61,7 +61,8 @@ const handleData = async (responseText, win) => { | |||
songInfo.album = data?.videoDetails?.album; // Will be undefined if video exist | |||
|
|||
const oldUrl = songInfo.imageSrc; | |||
songInfo.imageSrc = videoDetails.thumbnail?.thumbnails?.pop()?.url.split("?")[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't use pop
if not intending to change the original array
Wanted to use `.at(-1) but it requires node 16.6+ aka electron 23+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it has been released since then
https://www.electronjs.org/blog/electron-23-0
but it drops support for Windows 7/8/8.1, so I'm unsure when should we bump that
@@ -95,15 +96,15 @@ const registerProvider = (win) => { | |||
await handleData(responseText, win); | |||
handlingData = false; | |||
callbacks.forEach((c) => { | |||
c(songInfo); | |||
c(songInfo, "video-src-changed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused right now, but now every plugin has the option to know whats the reason for the songInfo callback
plugins/notifications/utils.js
Outdated
switch(config.get("toastStyle")) { | ||
case module.exports.ToastStyles.logo: | ||
case module.exports.ToastStyles.legacy: | ||
return this.saveImage(nativeImageToLogo(songInfo.image), tempIcon); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is inconsistent stylewise, I'm not sure which one should be used here
What do you think @th-ch ?😅
Should we use module.exports.
or this.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong preference, this
is fine :)
@th-ch any thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, merge is expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not test Windows specific features but looks good, thanks! ✅
@@ -131,16 +131,14 @@ const mainMenuTemplate = (win) => { | |||
], | |||
}, | |||
{ | |||
label: "Single instance lock", | |||
label: "Release single instance lock", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it seems the button will request the lock it it does not have it, name can be misleading
label: "Release single instance lock", | |
label: "Single instance lock", |
config = { ...config, ...options }; | ||
}; | ||
|
||
module.exports.setAndMaybeRestart = (option, value) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should we make the name more explicit? Sthg like
module.exports.setAndMaybeRestart = (option, value) => { | |
module.exports.setInMenu = (option, value) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think setAndMaybeRestart
is more explicit since the function name indicates that the difference is the restart option
setInMenu
doesn't tell me much tbh
plugins/notifications/utils.js
Outdated
switch(config.get("toastStyle")) { | ||
case module.exports.ToastStyles.logo: | ||
case module.exports.ToastStyles.legacy: | ||
return this.saveImage(nativeImageToLogo(songInfo.image), tempIcon); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong preference, this
is fine :)
This is a pretty big PR whose main goal is to replace SnoreToast/node-notifier with native toast notifications using XML
— along the way there were some other changes detailed below
Feel free to ask questions about code or its purpose in this PR.
Changes related only to this plugin:
Made config local to the plugin, which means it is synced between the menu and the other files
Different presets and option to hide text on buttons (screenshots below)
Adaptive text size in the
Banner Centered
toasts + Time widget in the one custom presetOption to open notification on tray click, which can act as a very cool quick music control from everywhere
(This feature is a personal favorite of mine 🤩)
Other Changes:
song-controls
actions)could capitalize on this for other plugins/features
For example, users could execute the URI with custom remote control apps
Multiple Instances break the Protocol Handler, (and already broke a few other features), and also resources cost is unreasonable
Should probably recolor them to white at some point
song-info-front
are now activated on demand instead of depending on options(Easier to use it from plugins now, don't have to modify song-info-front logic every time, don't have to think about if other plugins requested it since it's a singleton)
video-src-changed
orplayPaused
)Not actually used anywhere for now
Won't Do
(this plugin doesn't require a restart to apply those options)
Screenshots
Click me
xml_logo_ascii
✅This is how it looked on v1.19.0 (using SnoreToast), I see no reason not to include
xml_logo_icons_notext
❔The gap is unavoidable since there HAS to be a text field inside a button, looks better when not zoomed in on the toast
xml_logo_icons
✅xml_hero
✅Looks a bit weird because the hero appears on top of the notification (above the close button)
xml_banner_bottom
✅similar to xml_hero but less weird, I see no reason not to include
xml_banner_top_custom
❓this one is actually interesting, notice the special info at the right (Album name, Release year)
Cool concept but the top gap is pretty big…
xml_banner_centered_bottom
❓Both of the
centered
toasts make use of special text properties (like font, position)the text could be made a bit smaller, but the gap will remain
xml_banner_centered_top
❔I like the idea of a bigger font and centered position, but not sure if it's worth the topgap tradeoff
dev stuff
[Notifications Visualizer] --- [download appx] --- [CUSTOM PRESETS]
more on toast XML:
schema
general tutorial
toasttemplatetype