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 Watch2Gether support #2066

Merged
merged 9 commits into from
Oct 15, 2019
Merged

Add Watch2Gether support #2066

merged 9 commits into from
Oct 15, 2019

Conversation

jcdcdev
Copy link
Contributor

@jcdcdev jcdcdev commented Sep 27, 2019

Fix for Issue #2064

Added an entry in connectors.js to enable Watch2Gether support.

Additional context

Watch2Gether works by embedding a YouTube iframe.
The connectors/youtube-embed.js file works on this website in the same way it does on Discogs.

Watch2Gether works by embedding a YouTube iframe.

The `connectors/youtube-embed.js` file works on this website in the same way it does on Discogs.
@alexesprit alexesprit added connector This issue or pull request is related to connectors implemented This pull request proposes a new connector implemented labels Oct 5, 2019
@alexesprit
Copy link
Member

Does it work for you? It does not for me.

@inverse
Copy link
Member

inverse commented Oct 9, 2019

Thanks for contributing.. I tried to test this out but it's not working for me either.

@jcdcdev
Copy link
Contributor Author

jcdcdev commented Oct 9, 2019

I was woefully naive with this PR.

Not only does watch2gether support other players than just YouTube but it dynmically loads in an iframe (I don't believe this extension supports injecting the content script into dynamic iframes).

I'm looking into monkey patching some of the website's internal events to intercept player state change and track info change.

@jcdcdev
Copy link
Contributor Author

jcdcdev commented Oct 12, 2019

I've added a new connector that:

  • watches the play button and timer info elements for state change using a MutationObserver
  • gets track info from the chat box

@alexesprit
Copy link
Member

alexesprit commented Oct 13, 2019

Why did you wrap the code in a class?

Edit: I simplified the connector a little, here is the updated code.
Changes:

  • Remove class wrap;
  • Use timeInfoSelector;
  • Watch for changes of a one element;
  • Use selectors directly;
  • Add YouTube metadata filter;
  • Other stylistic changes.

@jcdcdev
Copy link
Contributor Author

jcdcdev commented Oct 14, 2019

After seeing your changes I can see that I over complicated my implementation.

Your refactor looks great!

@alexesprit
Copy link
Member

Could you push these changes? I'll merge this PR then.

@jcdcdev
Copy link
Contributor Author

jcdcdev commented Oct 14, 2019

I've pushed the changes and reintroduced the jQuery workaround to decode html entities (there's a bug in watch2gether that results in html entities showing in track names).

@alexesprit
Copy link
Member

alexesprit commented Oct 14, 2019

(there's a bug in watch2gether that results in html entities showing in track names).

Have you an example of such videos? I want to test if the metadata filter that replaces HTML entities will resolve the issue.

Edit: the idea is to append a new filter that will replace HTML entities:

Connector.applyFilter(
    MetadataFilter.getYoutubeFilter().appendFilters({
        all: MetadataFilter.decodeHtmlEntities
    })
);

This filter will replace HTML entities if track name is changed, but not on every DOM change.

@jcdcdev
Copy link
Contributor Author

jcdcdev commented Oct 14, 2019

Thank you for taking time to explain that to me.

I'd like to pick up a few other requests for connectors so I'll be sure to pay more attention to the docs and lint rules.

it looks as if watch2gether fixed this issue anyway (I've been testing against an old room, removing songs and adding them back in shows the right name now...)

@alexesprit
Copy link
Member

it looks as if watch2gether fixed this issue anyway (I've been testing against an old room, removing songs and adding them back in shows the right name now...)

Nice, so the PR is ready to merge, right? It looks good for me, btw.

@jcdcdev
Copy link
Contributor Author

jcdcdev commented Oct 14, 2019

Nice, so the PR is ready to merge, right?

Yup, it's all good to go!

@alexesprit alexesprit merged commit f97bfe8 into web-scrobbler:master Oct 15, 2019
@alexesprit alexesprit mentioned this pull request Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connector This issue or pull request is related to connectors implemented This pull request proposes a new connector implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants