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

Protocol handler arguments are not correctly parsed #1098

Closed
wants to merge 1 commit into from
Closed

Protocol handler arguments are not correctly parsed #1098

wants to merge 1 commit into from

Conversation

pathmann
Copy link

@pathmann pathmann commented Apr 2, 2023

The last character from a protocol handler argument is removed, so the argument won't be parsed correctly to call the requested song control function.

So executing youtubemusic://playPause will be parsed to playPaus, on the other hand executing youtubemusic://playPauseX will trigger the playPause song control.

@Araxeus
Copy link
Collaborator

Araxeus commented Apr 2, 2023

Hmm this is weird, it definitely works on my pc (Windows 10)

what OS are you on?

@pathmann
Copy link
Author

pathmann commented Apr 2, 2023

(Gentoo) Linux 64 bit

What is the purpose (maybe only on Windows) of removing that last character?

@Araxeus
Copy link
Collaborator

Araxeus commented Apr 2, 2023

Oh I see the problem now. on windows there is always an appended character - /

So we just need to check if it ends with /

image

btw we didn't catch this because the protocol-handler was originally added for interactive notifications which are windows only

Out of curiosity, might I ask what did you use the protocol handler for?
and also how did you know that the protocol handler exists?
and how did you understand the cause of the bug since it doesn't say anything when failing?😅

@pathmann
Copy link
Author

pathmann commented Apr 2, 2023

I see.

Long story: I needed a possibility to like the current song from my stream deck, so I searched the code and issues for a way and found #946.

Your comment stated youtubemusic://pause as the way to go, so I thought this would be a bug, but I could also live with appending some suffix (like /)

@Araxeus
Copy link
Collaborator

Araxeus commented Apr 2, 2023

The code just have to check if it ends with / like I said, this will fix all problems

If you don't want / can't do it yourself, just tell me and ill gladly open a PR

(btw you make me glad that I spend so much effort on PR descriptions ☺️)

@pathmann
Copy link
Author

pathmann commented Apr 2, 2023

If you don't mind, sure. I'm not very experienced in Javascript 😁

@Araxeus
Copy link
Collaborator

Araxeus commented Apr 2, 2023

Ok FYI next time you fork a repo to open a Pull Request – create a new branch first, don't open PR's from your default branch

@Araxeus
Copy link
Collaborator

Araxeus commented Apr 2, 2023

I've opened a PR #1099 can you try it out?

just check this branch out https://github.com/Araxeus/youtube-music/tree/fix-protocol-handler-on-unix

@pathmann
Copy link
Author

pathmann commented Apr 2, 2023

Yep, works.

Thanks a lot

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