YTMD/youtube-music
1
0
Fork 0
Code Issues 445 Pull requests 34 Projects Releases 87 Packages Wiki Activity Actions

WIP: [DEPRECATED] Chromecast Plugin #296

Draft
Araxeus wants to merge 17 commits from Araxeus/chromecast into master
pull from: Araxeus/chromecast
merge into: YTMD:master
Conversation 10 Commits 17 Files changed 9 +409 -25
Araxeus commented 2021-05-23 15:48:37 +00:00 (Migrated from github.com)
Copy link

Implement and closes #289

This plugin connects to all nearby Chromecast devices and stream youtube music with video

Sync Options:

  • Seek - sync current time on Chromecast when current time is changed in local video
  • StartTime - wait for Chromecast to start playback before local playback begins
  • Volume - self explanatory

Features Tested by @playday3008 - who originally requested this plugin

I can't see why snyk failed the security test (access error - not a member of organization)

Implement and closes #289 This plugin connects to all nearby Chromecast devices and stream youtube music with video Sync Options: * Seek - sync current time on Chromecast when current time is changed in local video * StartTime - wait for Chromecast to start playback before local playback begins * Volume - self explanatory Features Tested by @playday3008 - who originally requested this plugin > I can't see why snyk failed the security test (access error - not a member of organization)
👍 6 👀 1
th-ch (Migrated from github.com) reviewed 2021-06-19 20:55:52 +00:00
th-ch (Migrated from github.com) left a comment
Copy link

Thanks for the addition! Did not test it with a real chromecast device yet, but looks promising! Left a few first comments - about the security issue reported by Snyk, chromecast-api@0.3.4 introduces dns-packet@4.2.0 which has a Remote Memory Exposure vulnerability, fixed in dns-packet@5.2.4. Until the chromecast package has an update for this, the version can be forced with a resolution "dns-packet": "5.2.4", in package.json:
1fc8f7ad7e/package.json (L101)

(note that it's not the same major version so worth manually testing that everything works correctly!)

Thanks for the addition! Did not test it with a real chromecast device yet, but looks promising! Left a few first comments - about the security issue reported by Snyk, `chromecast-api@0.3.4` introduces `dns-packet@4.2.0` which has a [`Remote Memory Exposure` vulnerability](https://snyk.io/vuln/SNYK-JS-DNSPACKET-1293563), fixed in `dns-packet@5.2.4`. Until the chromecast package has an update for this, the version can be forced with a resolution `"dns-packet": "5.2.4",` in package.json: https://github.com/th-ch/youtube-music/blob/1fc8f7ad7eeda8dd342f910e6df4a859c96fcd60/package.json#L101 _(note that it's not the same major version so worth manually testing that everything works correctly!)_
plugins/chromecast/back.js
@ -0,0 +86,4 @@
function transformURL(url) {
const videoId = url.match(/(?:http(?:s?):\/\/)?(?:www\.)?(?:music\.)?youtu(?:be\.com\/watch\?v=|\.be\/)([\w\-\_]*)/);
// videoId[1] should always be valid since regex should always be valid - rickroll video should never happen :)
return "https://youtube.com/watch?v=" + (videoId.length > 1 ? videoId[1] : "dQw4w9WgXcQ");
th-ch (Migrated from github.com) commented 2021-06-19 20:33:12 +00:00
Copy link

Nice Rickroll 🙃 Maybe worth a comment in code, I wondered why this hardcoded ID was there at first 😅

Nice Rickroll 🙃 Maybe worth a comment in code, I wondered why this hardcoded ID was there at first 😅
providers/song-controls-front.js
@ -1,18 +1,37 @@
const { ipcRenderer } = require("electron");
th-ch (Migrated from github.com) commented 2021-06-19 20:40:38 +00:00
Copy link

nit: it might not be necessary to define the 2 inner functions if they are a one-liner and only used once!

nit: it might not be necessary to define the 2 inner functions if they are a one-liner and only used once!
th-ch (Migrated from github.com) commented 2021-06-19 20:42:02 +00:00
Copy link

nit: it can be simplified into

function checkVideo() {
    if (!video) {
        video = document.querySelector("video");
    }

    return !!video;
}
nit: it can be simplified into ```js function checkVideo() { if (!video) { video = document.querySelector("video"); } return !!video; } ```
th-ch (Migrated from github.com) commented 2021-06-19 20:43:32 +00:00
Copy link

nit: could also be the default case for the switch, to be defensive

nit: could also be the default case for the switch, to be defensive
providers/song-controls.js
th-ch (Migrated from github.com) commented 2021-06-19 20:38:38 +00:00
Copy link

I'm getting a Error: Failed to serialize arguments (song-controls.js:15:54) when using the play/pause media key, is it expected?

I'm getting a `Error: Failed to serialize arguments` (`song-controls.js:15:54`) when using the play/pause media key, is it expected?
providers/song-controls.js
@ -13,7 +13,8 @@ module.exports = (win) => {
previous: () => pressKey(win, "k"),
next: () => pressKey(win, "j"),
playPause: () => win.webContents.send("playPause"),
th-ch (Migrated from github.com) commented 2021-06-19 20:42:30 +00:00
Copy link

Is it expected this one is removed?

Is it expected this one is removed?
Araxeus (Migrated from github.com) reviewed 2021-06-19 21:42:27 +00:00
providers/song-controls-front.js
@ -1,18 +1,37 @@
const { ipcRenderer } = require("electron");
Araxeus (Migrated from github.com) commented 2021-06-19 21:42:27 +00:00
Copy link

done 👍🏼

done 👍🏼
Araxeus (Migrated from github.com) reviewed 2021-06-19 21:42:29 +00:00
providers/song-controls.js
@ -13,7 +13,8 @@ module.exports = (win) => {
previous: () => pressKey(win, "k"),
next: () => pressKey(win, "j"),
playPause: () => win.webContents.send("playPause"),
Araxeus (Migrated from github.com) commented 2021-06-19 21:42:29 +00:00
Copy link

It actually has nothing to do with this pull - but the like function was written twice - why's that?
e0cb132686/providers/song-controls.js (L28)
e0cb132686/providers/song-controls.js (L16)

I just assumed it was a mistake since the first one works - so I removed the second one

It actually has nothing to do with this pull - but the `like` function was written twice - why's that? https://github.com/th-ch/youtube-music/blob/e0cb1326868594d172668712da8581b893c925ec/providers/song-controls.js#L28 https://github.com/th-ch/youtube-music/blob/e0cb1326868594d172668712da8581b893c925ec/providers/song-controls.js#L16 I just assumed it was a mistake since the first one works - so I removed the second one
Araxeus (Migrated from github.com) reviewed 2021-06-19 21:42:32 +00:00
providers/song-controls-front.js
@ -1,18 +1,37 @@
const { ipcRenderer } = require("electron");
Araxeus (Migrated from github.com) commented 2021-06-19 21:42:31 +00:00
Copy link

done 👍🏼

done 👍🏼
Araxeus (Migrated from github.com) reviewed 2021-06-19 21:43:13 +00:00
providers/song-controls-front.js
@ -1,18 +1,37 @@
const { ipcRenderer } = require("electron");
Araxeus (Migrated from github.com) commented 2021-06-19 21:43:12 +00:00
Copy link

I agree that play() is unnecessary, but pause() is called in 2 different places

(I originally added play() only because pause() was there - but I removed it as requested)

I agree that `play()` is unnecessary, but `pause()` is called in 2 different places (I originally added `play()` only because `pause()` was there - but I removed it as requested)
Araxeus (Migrated from github.com) reviewed 2021-06-19 21:43:22 +00:00
providers/song-controls.js
Araxeus (Migrated from github.com) commented 2021-06-19 21:43:22 +00:00
Copy link

It happened because shortcut plugin call the method with win.webContents argument.
I just fixed it by making play, pause directly part of song controls
sorry about that

It happened because shortcut plugin call the method with `win.webContents` argument. I just fixed it by making `play, pause` directly part of song controls sorry about that
Araxeus (Migrated from github.com) reviewed 2021-06-19 21:43:50 +00:00
plugins/chromecast/back.js
@ -0,0 +86,4 @@
function transformURL(url) {
const videoId = url.match(/(?:http(?:s?):\/\/)?(?:www\.)?(?:music\.)?youtu(?:be\.com\/watch\?v=|\.be\/)([\w\-\_]*)/);
// videoId[1] should always be valid since regex should always be valid - rickroll video should never happen :)
return "https://youtube.com/watch?v=" + (videoId.length > 1 ? videoId[1] : "dQw4w9WgXcQ");
Araxeus (Migrated from github.com) commented 2021-06-19 21:43:50 +00:00
Copy link

There is a comment already 😅
I thought the module would update its version faster - and this method will be unneeded
but sadly it hasn't happened yet - the fix pr was merged but the npm version wasn't updated yet

(method should only be called when songInfo is updated which means the url regex is valid - which means you should never get rick-rolled 😝)

There is a comment already 😅 I thought the module would update its version faster - and this method will be unneeded but sadly it hasn't happened yet - the fix pr was merged but the npm version wasn't updated yet (method should only be called when songInfo is updated which means the url regex is valid - which means you should never get rick-rolled 😝)
Araxeus commented 2021-06-21 15:51:13 +00:00 (Migrated from github.com)
Copy link

(note that it's not the same major version so worth manually testing that everything works correctly!)

Tested with latest changes

> _(note that it's not the same major version so worth manually testing that everything works correctly!)_ Tested with latest changes
playday3008 commented 2021-06-24 06:55:21 +00:00 (Migrated from github.com)
Copy link

Well, we still need device list with optional auto connection

Well, we still need device list with optional auto connection
danir-de commented 2024-01-05 20:50:38 +00:00 (Migrated from github.com)
Copy link

This feature would make the app feature complete for me, sadly I cannot help by coding. If there is anything I can do to help to refactor this for the latest version, please let me know!

This feature would make the app feature complete for me, sadly I cannot help by coding. If there is anything I can do to help to refactor this for the latest version, please let me know!
This pull request has changes conflicting with the target branch.
  • config/defaults.js
  • package.json
  • plugins/taskbar-mediacontrol/back.js
  • providers/song-controls-front.js
  • providers/song-controls.js
  • yarn.lock
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin Araxeus/chromecast:Araxeus/chromecast
git switch Araxeus/chromecast

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch master
git merge --no-ff Araxeus/chromecast
git switch Araxeus/chromecast
git rebase master
git switch master
git merge --ff-only Araxeus/chromecast
git switch Araxeus/chromecast
git rebase master
git switch master
git merge --no-ff Araxeus/chromecast
git switch master
git merge --squash Araxeus/chromecast
git switch master
git merge --ff-only Araxeus/chromecast
git switch master
git merge Araxeus/chromecast
git push origin master
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
awaiting-reply

Awaiting reply

  
breaking changes

   
bug

Something isn't working

  
cannot-reproduce

Bug cannot be reproduced

  
dependencies

Pull requests that update a dependency file

  
documentation

   
duplicate

This issue or pull request already exists

  
electron-issue

It's an Electron or Electron related dependencies issue (not YTM-Desktop issue)

  
enhancement

New feature or request

  
fix-available

A fix to the issue is available in a new version

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
javascript

Pull requests that update javascript code

  
need more information

Need more information about the issue

  
need rebase

This plugin need rebase

  
official-youtube-music-issue

It's an YouTube's issue (not YTM-Desktop issue)

  
plugin request

An issue/discussion requesting for a new plugin or new functionality to an existing plugin.

  
question

Further information is requested

  
release

Release

  
security

   
stale

   
Status: blocked

There are something to do before this

  
typo

   
wontfix

This will not be worked on

  
ytmd-issue

An issue/discussion about ytm desktop, unrelated to th-ch/youtube-music

No labels
awaiting-reply
breaking changes
bug
cannot-reproduce
dependencies
documentation
duplicate
electron-issue
enhancement
fix-available
good first issue
help wanted
invalid
javascript
need more information
need rebase
official-youtube-music-issue
plugin request
question
release
security
stale
Status: blocked
typo
wontfix
ytmd-issue
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: YTMD/youtube-music#296
No description provided.
Delete branch "Araxeus/chromecast"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?