Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

review our shortcut logic. (bug: screenshot by shortcut might takes several screenshots unintentionally; etc: ... #1565) #1770

Open
ImprovedTube opened this issue Sep 16, 2023 · 6 comments
Assignees
Labels
bounty Will pass on donations (Optional) - (OR: Requester will pay personally. Only if stated!) Bug Bug or required update after YouTube changes good first issue A GitHub standard for inviting (new) contributors *Congratulations in advance!* help wanted Just an old github standard we add automatically. (The team can remove it when working on it.) up-for-grabs (a github standard for inviting new contributors) - Welcome! ♥

Comments

@ImprovedTube
Copy link
Member

ImprovedTube commented Sep 16, 2023

might be easy considering the relevance.

https://github.com/code-charity/youtube/blob/master/js%26css/web-accessible/www.youtube.com/shortcuts.js

@ImprovedTube ImprovedTube added Bug Bug or required update after YouTube changes help wanted Just an old github standard we add automatically. (The team can remove it when working on it.) good first issue A GitHub standard for inviting (new) contributors *Congratulations in advance!* up-for-grabs (a github standard for inviting new contributors) - Welcome! ♥ important Critical or common. Thus to prioritize labels Sep 16, 2023
@0scvr
Copy link

0scvr commented Oct 1, 2023

Is this bug still present ? If not , I would be willing to take a look.

@ImprovedTube
Copy link
Member Author

hi! @0scvr wow, that would be great! regret i missed it!!

#1565

@0scvr
Copy link

0scvr commented Oct 21, 2023

I've tested this bug locally and I do not have this issue. When pressing the shortcut for a short time and immediately releasing the keys, only 1 image is downloaded. If I stay pressed for longer though, multiple images will be downloaded. I think this is working correctly, no?

@ImprovedTube ImprovedTube changed the title review our shortcut logic. bug: screenshot by shortcut takes several screenshots (for the shortest possible keypress) review our shortcut logic. (bug: screenshot by shortcut might takes several screenshots unintentionally; etc: ... #1565) Oct 21, 2023
@ImprovedTube
Copy link
Member Author

ImprovedTube commented Oct 23, 2023

hi @0scvr, yes, admittedly it was bad only when is used a bit worn down key and a fast CPU.

  • However some of the screenshots will easily be the same frame... (=bad*)

maybe faster than the rate set in the OS for repeating through holding (which might be a hint, that there might be bad loop in the code. And the same logic applies to all our shortcuts, which makes it relevant / exciting #1565 )


( if a user ever wanted to screenshot every frame, this could be done while the video is paused. While every hit of the shortcut could also skip the current frame (just like our https://github.com/code-charity/frame-by-frame) )

@0scvr
Copy link

0scvr commented Oct 23, 2023

I don't think I'm the person for this issue, sorry.

@0scvr 0scvr removed their assignment Oct 23, 2023
@ImprovedTube ImprovedTube added the bounty Will pass on donations (Optional) - (OR: Requester will pay personally. Only if stated!) label Nov 5, 2023
@ImprovedTube ImprovedTube added the Rare use-case? Rare / Niche (or just for fun?) Or to become a remainder replaced by a successor or anything. label Nov 25, 2023
@ImprovedTube ImprovedTube removed Rare use-case? Rare / Niche (or just for fun?) Or to become a remainder replaced by a successor or anything. important Critical or common. Thus to prioritize labels Feb 6, 2024
@ImprovedTube ImprovedTube assigned 0scvr and unassigned 0scvr Apr 11, 2024
@raszpl
Copy link
Contributor

raszpl commented Jun 29, 2024

Imo a non issue. Everything is working correctly, shortcuts are triggered at the rate of keyboard events coming in. Imo adding artificial delays (lowers responsiveness) just in case someone could have broken keyboard is a bad idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty Will pass on donations (Optional) - (OR: Requester will pay personally. Only if stated!) Bug Bug or required update after YouTube changes good first issue A GitHub standard for inviting (new) contributors *Congratulations in advance!* help wanted Just an old github standard we add automatically. (The team can remove it when working on it.) up-for-grabs (a github standard for inviting new contributors) - Welcome! ♥
Projects
Status: No status
Development

No branches or pull requests

3 participants