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
43 stars 9 forks source link

Add an option to copy screenshot to clipboard #22

Closed joggee-fr closed 2 years ago

joggee-fr commented 2 years ago

Use a background script companion to access clipboard. Note the selected API browser.clipboard.setImageData() is only supported by Firefox. Also add a notification to indicate copy success.

Linked to issue #8.

gurumukhi commented 2 years ago

Looks good, I will test & merge it by the weekend or in next week. Thanks a lot for helping with this

gurumukhi commented 2 years ago

Noticed few issues, lets test it more.

One issue I noticed was addition of duplicate button on disabling & enabling the add-on. It can be avoided by simply adding something like this -

  if(document.querySelector('button.ytp-screenshot')) {
    logger('Button already added');
    return;
  }

Apart from this, I was able to test copy to clipboard feature initially, but after a while it started breaking. I dont know exact issue but we can test it further to see if we find any other issue.

We should also specify this on the options page itself - 'Reload the Youtube page to apply these settings'. In case user change button action & tries it without reloading Youtube page it wont work.

Thanks :)

joggee-fr commented 2 years ago

One issue I noticed was addition of duplicate button on disabling & enabling the add-on. It can be avoided by simply adding something like this -

  if(document.querySelector('button.ytp-screenshot')) {
  logger('Button already added');
  return;
  }

You're right. As this is a different issue, I will try to open an other PR to fix it with a slightly modified patch.

PRIXYY commented 2 years ago

I can try the updated version and see if I notice any issues.

joggee-fr commented 2 years ago

@PRIXYY, thanks for your help. I will update soon my branch at least rebasing to master.

joggee-fr commented 2 years ago

After rebase and quick tests, I did not notice any issue yet. @PRIXYY, @gurumukhi, let me know if you detect any bug after trying.

I agree adding "Reload the Youtube page to apply these settings" in settings page would be a nice feature. However, I think we can postpone it to another next PR.

joggee-fr commented 2 years ago

Any update?

PRIXYY commented 2 years ago

I have tested it. The 'copy to clipboard' feature is working properly.

gurumukhi commented 2 years ago

Great, I will merge it.

On Wed, 26 Oct, 2022, 19:45 PRIXYY, @.***> wrote:

I have tested it. The 'copy to clipboard' feature is working properly.

— Reply to this email directly, view it on GitHub https://github.com/gurumukhi/youtube-screenshot/pull/22#issuecomment-1292120528, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAU7ELK6WYQON6PTS7W3EU3WFE4GPANCNFSM6AAAAAAQ37LHK4 . You are receiving this because you were mentioned.Message ID: @.***>

gurumukhi commented 2 years ago

Thanks a lot @joggee-fr and @PRIXYY