[DEPRECATED] Chromecast Plugin #296
Conversation
There was a problem hiding this comment.
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, [email protected] introduces [email protected] which has a Remote Memory Exposure vulnerability, fixed in [email protected]. 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:
Line 101 in 1fc8f7a
(note that it's not the same major version so worth manually testing that everything works correctly!)
| function transformURL(url) {// will not be needed after https://github.com/alxhotel/chromecast-api/pull/69 (chromecastAPI v0.3.5) | ||
| 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 | ||
| return "https://youtube.com/watch?v=" + (videoId.length > 1 ? videoId[1] : "dQw4w9WgXcQ"); |
There was a problem hiding this comment.
Nice Rickroll 🙃 Maybe worth a comment in code, I wondered why this hardcoded ID was there at first 😅
There was a problem hiding this comment.
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 😝)
providers/song-controls.js
Outdated
| @@ -12,8 +12,7 @@ module.exports = (win) => { | |||
| // Playback | |||
| previous: () => pressKey(win, "k"), | |||
| next: () => pressKey(win, "j"), | |||
| playPause: () => win.webContents.send("playPause"), | |||
| like: () => pressKey(win, "_"), | |||
| playPause: (toPlay = undefined) => win.webContents.send("playPause", toPlay), | |||
There was a problem hiding this comment.
I'm getting a Error: Failed to serialize arguments (song-controls.js:15:54) when using the play/pause media key, is it expected?
There was a problem hiding this comment.
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
providers/song-controls-front.js
Outdated
| function play() { | ||
| video.play(); | ||
| } | ||
|
|
||
| function pause() { |
There was a problem hiding this comment.
nit: it might not be necessary to define the 2 inner functions if they are a one-liner and only used once!
There was a problem hiding this comment.
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)
providers/song-controls-front.js
Outdated
| if (!video) { | ||
| video = document.querySelector("video"); | ||
| if (!video) { | ||
| return false; | ||
| } | ||
| }); | ||
| }; | ||
| } return true; |
There was a problem hiding this comment.
nit: it can be simplified into
function checkVideo() {
if (!video) {
video = document.querySelector("video");
}
return !!video;
}
providers/song-controls-front.js
Outdated
| if (!checkVideo()) return; | ||
|
|
||
| switch(toPlay) { | ||
| case undefined: |
There was a problem hiding this comment.
nit: could also be the default case for the switch, to be defensive
Tested with latest changes |
|
Well, we still need device list with optional auto connection |
JellyBrick
added
need rebase
|
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! |
Implement and closes #289
This plugin connects to all nearby Chromecast devices and stream youtube music with video
Sync Options:
Features Tested by @playday3008 - who originally requested this plugin