code-charity / youtube

[top~1 open YouTube & Video web-extension] Enrich your experience & choice! 🧰100+clever features📌set&forget📌Longest-standing(yet rare&tough alone. Please help/join🧩us👨‍👩‍👧‍👧) ..⋮ {playback|content discovery|player|extra buttons|distractions|related videos|shorts|ads|quality|codec|full tab|full screen}
http://improvedtube.com
Other
3.29k stars 498 forks source link

playlist.js undefined 'playlistData' proper fix, even safer #2387

Closed raszpl closed 1 week ago

raszpl commented 1 week ago

https://github.com/code-charity/youtube/issues/1720 checks all variables/properties it touches for max safety

ImprovedTube commented 1 week ago

why not start any feature with if ( storage..., so that a feature error cant happen to everyone?


and this feels like over-optimization? https://github.com/code-charity/youtube/blob/035b24fd9290f570ae5beef6cdcbbeb5a063b52c/js%26css/web-accessible/core.js#L180


why not wait for the playlist?

raszpl commented 1 week ago
var waitForPlaylist = setInterval(() => { if (this.elements.ytd_watch.playlistData || (++tries >= maxTries) ) {
        this.elements.ytd_watch.playlistData.currentIndex = this.elements.ytd_watch.playlistData.totalVideos; clearInterval(waitForPlaylist );}         
        intervalMs *= 1.4;}, intervalMs); }} catch (error) { console.error("Waiting for playlist", error);}

no no nooo, my poor patch :( not this abomination again, you even pasted same broken code again. SetInterval is its own new scope, this is Window there, this always crashes.

why not start any feature with if ( storage...,

so it can handle live option switching without specialized entries in core.js

so that a feature error cant happen to everyone?

it already cant happen with my patch, checks every variable before touching anything

and this files like over-optimization?

https://github.com/code-charity/youtube/blob/035b24fd9290f570ae5beef6cdcbbeb5a063b52c/js%26css/web-accessible/core.js#L180

dont understand what you mean? CSS cant overwrite .currentIndex

why not wait for the playlist?

because https://github.com/code-charity/youtube/issues/1720#issuecomment-2171693368 With current autoplay implementation playlistUpNextAutoplay is being called by playerOnPause at least 2 times (up to 4 :o), with first 1-2 before page even started rendering. There is no need to try forcing anything on first call because the ONLY playlistUpNextAutoplay call we are really interested in happens at the end of video when player pauses just before deciding which next video to play from the playlist. This can be handled solely by playerOnEnded, call from playerOnPause can be removed entirely.

raszpl commented 1 week ago

This can be handled solely by playerOnEnded, call from playerOnPause can be removed entirely.

Nope. Firefox doesnt trigger this.addEventListener('ended', ImprovedTube.playerOnEnded, true); Its fully supported and standard https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/ended_event so most likely YT player is doing something weird, like manually pausing at the end maybe. Safer to leave it in both playerOnEnded/playerOnPause

ImprovedTube commented 1 week ago

hi @raszpl,

so it can handle live option switching without specialized entries in core.js

how are the same lines more specialized when moved to their spot? (While most of our features are meant to set & forget, the feedback core.js#L180-L.. is just an extra optional live-UX convenience. Where later we might also add an animation/pointer to the spot on the page that changes.

over-optimization

Something might be switched-on once only and switched-off once only by only years later by a fraction of the users who switched it on previously. So our many toggles don't need to run more than 1 lines of code for every user every time. Or 0 lines if their functions could be called like "for each enabled feature in storage )(...yet of course many of those lines can run a billion times per second so no force this "set & forget-structure of development)