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

Add support for YouTube Music. #31

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pedromakaveli
Copy link

@pedromakaveli pedromakaveli commented Aug 13, 2023

added new support for youtube music

Copy link
Owner

@guihkx guihkx left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! 🏆

Based on an initial impression, the logic looks good overall. But there are minor issues that'd I like you to fix before I can finally test and merge this, though. :)

Most issues are related to code formatting, so if you want to easily fix those, please check out the "Contributing" section:

https://github.com/guihkx/spotishush#contributing

Also, can you please rename the pull request and mention that you're adding support for YouTube Music? Thanks!

src/spotishush.js Outdated Show resolved Hide resolved
src/spotishush.js Outdated Show resolved Hide resolved
src/spotishush.js Outdated Show resolved Hide resolved
src/spotishush.js Outdated Show resolved Hide resolved
src/spotishush.js Outdated Show resolved Hide resolved
src/spotishush.js Outdated Show resolved Hide resolved
src/spotishush.js Outdated Show resolved Hide resolved
src/spotishush.js Outdated Show resolved Hide resolved
@pedromakaveli pedromakaveli force-pushed the pedromakaveli-patch-1 branch 2 times, most recently from 5b12c48 to 4e363bf Compare August 16, 2023 04:43
@pedromakaveli pedromakaveli changed the title Pedromakaveli patch 1 Add support for YouTube Music. Aug 16, 2023
@guihkx guihkx force-pushed the pedromakaveli-patch-1 branch from 4e363bf to 2845fea Compare September 29, 2023 18:22
@guihkx
Copy link
Owner

guihkx commented Sep 29, 2023

A minor (yet annoying) issue I found with the current implementation is that if you're unlucky enough to get an ad the first time you play a song, the extension will not mute it fast enough:

Video_2023-09-27_01-43-21_edit.mp4

So, I'd like to find a faster way to handle this specific scenario before I can merge this.

Other than that, I'd say the detection mechanism seems to work pretty reliably!

@guihkx guihkx marked this pull request as draft December 7, 2023 19:56
@gdelcroix
Copy link

i think the way to handle this more quickly would be to mute YoutubeMusic by default immediately when starting the monitoring, and unmute if the first video isn't an ad.

@guihkx
Copy link
Owner

guihkx commented Nov 21, 2024

Hmm, I'm not sure if that would be preferable. In a scenario where we don't get an ad on the first music video, then the music itself would be muted for a second, which IMO would be equally annoying.

I haven't looked at this in a while too, (I don't really use YouTube Music), but perhaps the delay in muting/unmuting is gone now. Or the ad detection mechanism is simply broken... Either way, I have to revisit this.

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.

3 participants