add preview thumbnails - Githubissues

user234683 / youtube-local

browser-based client for watching Youtube anonymously and with greater page performance

GNU Affero General Public License v3.0

501 stars 62 forks source link

add preview thumbnails #119

Closed zrose584 closed 2 years ago

user234683 commented 2 years ago

It's great, just a couple of stylistic and minor issues:

Cryptic and single letter variable names, making it hard to understand the code: l, url1 (what's the real difference between url and url1?), q, r, t, ts. Example suggestions: t -> board_params, l -> boards, url1 -> base_url
The line info['storyboard_spec_url'] = sb['playerStoryboardSpecRenderer']['spec'] if sb else None should use the deep_get function in case the 'spec' key isn't present, so we don't get an exception if YouTube changes playerStoryboardSpecRender's keys. Which will also obviate the need for the if sb else None part

j4ln commented 2 years ago

The storyboard url needs adding to the embed.html template for video embeds to use.

zrose584 commented 2 years ago

Thank you for the review, I have changed or explained the variable names a bit. However I left t, its just a temporary tuple, whose fields get deconstructed in the next line. Hope that's fine!

so we don't get an exception if YouTube changes playerStoryboardSpecRender's keys

But wouldn't it be better to report such errors (throw a KeyError) as soon as possible? I mean I see how it would be useful to keep the app running when only minor features would break, but it also creates the possibility for a only "half working" app, which might frustrate the user?

user234683 commented 2 years ago

But wouldn't it be better to report such errors (throw a KeyError) as soon as possible? I mean I see how it would be useful to keep the app running when only minor features would break, but it also creates the possibility for a only "half working" app, which might frustrate the user?

Preventing the app from breaking is far more important since updates to youtube-local are slow, especially when I'm busy with uni. Additionally, it's more frustrating overall to have the page totally break, and to have to install updates frequently. In either case the frustrated user would submit a bug report, but in the minor-non-critical-feature-is-broken case the frustration would be less.

zrose584 commented 2 years ago

Ok, I changed the line

In either case the frustrated user would submit a bug report, but in the minor-non-critical-feature-is-broken case the frustration would be less.

But some videos just done have a storyboard, so the user can't distinguish between error and missing storyboard. This is actually exactly what happened to me and the subtitles: It took me quite some time to realize that its youtube-local not working correctly and not the videos having no subtitles.

What do you think about a general (opt in) debug mode, in which special 'assert' statements become active?

user234683 commented 2 years ago

Oh sorry one more thing, don't forget to make it work in the embed page as umimaso said

What do you think about a general (opt in) debug mode, in which special 'assert' statements become active?

I would be fine with that

zrose584 commented 2 years ago

Ok, done.

Just out of curiosity: Wouldn't you in theory be able to add such small (but important) adjustments yourself? I think I have enabled "Allow edits by maintainers".

user234683 commented 2 years ago

Ok, done.

Just out of curiosity: Wouldn't you in theory be able to add such small (but important) adjustments yourself? I think I have enabled "Allow edits by maintainers".

Yes in June but I'm too busy atm to do anything that takes over 15 min. Would need to familiarize with the code, understand, and test it