Add Screenshot button on Youtube Shorts

gurumukhi / youtube-screenshot

Cross browser addon for youtube video screenshots

https://addons.mozilla.org/en-US/firefox/addon/youtube-screenshot-button/

Mozilla Public License 2.0

41 stars 8 forks source link

Add Screenshot button on Youtube Shorts #30

Closed joggee-fr closed 1 year ago

joggee-fr commented 1 year ago

Main feature in this PR is adding Screenshot button to Youtube Shorts. Some minor fixes are also included here including one for issue #29.

Opened as draft first to start the discussion. Main concern is about MutationObserver now always on.

joggee-fr commented 1 year ago

Just pinging the last contributors (testing or dev) if they can take a look at this PR:

@thepwrtank18
@tacohitbox
@PRIXYY
@RyutoSetsujin

gurumukhi commented 1 year ago

Code looks good to me, need to do rigorous testing.

Do you think we should try to streamline addButtonOnRegularPlayer & addButtonOnShortsPlayer methods?

One way to do this is by writing a single method with params, but that will have multiple if-else blocks I think.
Another way is by taking out common functionalities if that really makes code more maintainable.

joggee-fr commented 1 year ago

Do you think we should try to streamline addButtonOnRegularPlayer & addButtonOnShortsPlayer methods?

You're right. Start and end of this two functions are analogue or similar and may be factorized.

joggee-fr commented 1 year ago

@gurumukhi If possible during your tests, could you check if the patch have an impact on page loading performance also? Thanks.

gurumukhi commented 1 year ago

@PRIXYY can you help in testing? You will need to download the code as zip from https://github.com/joggee-fr/youtube-screenshot/tree/jg/shorts for that.

joggee-fr commented 1 year ago

@gurumukhi, I have factorized the functions adding the screenshot button and done a bit of tests to check it. Seems OK. I am now considering this PR as ready for review 🙂 . Let me know if you find any issue during your tests. Thanks.

gurumukhi commented 1 year ago

This looks amazing, thanks a lot @joggee-fr :)

It looks like at some point we missed logger method's implementation (https://github.com/joggee-fr/youtube-screenshot/blob/jg/shorts/content_script.js#L3) during refactoring, can you look into this? I will merge the PR after that and release another version.

joggee-fr commented 1 year ago

@gurumukhi, this commit tries to change the way logger() is managed to avoid always checking if a global variable is set or not. It is really a minor change and we can drop it.

The only thing I am less comfortable with is I only had to edit the global variable at top of file before to force logging during dev and debug. However, a new global variable can always be added to force logger() to output in console even if the dedicated add-on preference is not set.

gurumukhi commented 1 year ago

Sure, merging it. 👍