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

Interactive notifications for windows #228

Merged
merged 18 commits into from
May 2, 2021

Conversation

Araxeus
Copy link
Collaborator

@Araxeus Araxeus commented Apr 8, 2021

This PR adds an option to 'notifications' plugin (only on windows)

Currently looks like this:
2-2


Could be changed to 2nd option which might be better:
1-1
Unicode: [\u{2770}-previous] [\u{25B6}-play] [\u{2771}-next]
(would need a subtle change of code since output for that play button [뿯▽] isn't the same as input for some reason)


The same lib could be used to setup interactive notification for mac, if anyone wants to work on that - Node Notifier

  • The standard pause button unicode wasn't used, because windows render those media buttons as blue

  • Because of a bug with the delivery method, appID cannot be set without breaking the button functionality
    (it makes returned data equal undefined)
    so appID will it will always be the default "SnoreToast".

Misc:

This PR also changes things for both notifications (normal ones too):

  • Song Icon is resized and cropped to keep original aspect ratio (resizing was really ugly with wide images before)

  • Made Notification Priority menu item show only on linux (since it works only on linux)

  • SongInfoProvider was parsing artist name based on video request , not on song - resulting in buggy artist names.
    changed it scrape artist name from playbar (native artist name)

    • could move functionality to notifications plugin instead of infoProvider
    • could be tweaked to wait a second on first run (first song always returns old VideoArtistName because playBar hasn't loaded by the time it queries it)
  • win.once() instead of win.on fix multiple callbacks being registered

Copy link
Owner

@th-ch th-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok, thanks for the contribution! 2 small nits (feel free to address them), otherwise it should be good to merge!

Copy link
Owner

@th-ch th-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the contribution!

@th-ch th-ch merged commit 472462c into th-ch:master May 2, 2021
@Araxeus Araxeus deleted the interactive-notifications branch May 8, 2021 21:09
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.

2 participants