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

Update shortcuts.js handlers rewrite, multiple fixes #2421

Merged
merged 30 commits into from
Jun 30, 2024
Merged

Update shortcuts.js handlers rewrite, multiple fixes #2421

merged 30 commits into from
Jun 30, 2024

Conversation

raszpl
Copy link
Contributor

@raszpl raszpl commented Jun 29, 2024

Fixes:

Rewrote: shortcutQuality shortcutIncreasePlaybackSpeed shortcutDecreasePlaybackSpeed shortcutIncreaseVolume shortcutDecreaseVolume

Rewrote non working: shortcutRotateVideo shortcutToggleAutoplay

Reformatted: shortcutToggleCards

Fixed: shortcutToggleLoop

Safer: shortcutPictureInPicture shortcutPlayPause shortcutStatsForNerds

Simplified: shortcutStop shortcutSubscribe shortcutResetPlaybackSpeed shortcutGoToSearchBox shortcutActivateFullscreen shortcutLike shortcutDislike shortcutSeekBackward shortcutSeekForward

Fixed bugs:

Problems:

  • "My active features" cant distinguish between default YT shortcut or User override using same key combination.

Testing:
Doesnt crash on load in Chrome/Vivaldi/FF so thats something :P
Tested ~30% of shortcuts in Chrome. Your turn to find bugs in it.

raszpl added 18 commits June 29, 2024 11:00
…step' to 'shortcuts_volume_step', 'shortcuts_playback_speed_step' to not mix those keys with real Shortcuts.

rename 'shortcut_volume_step', 'shortcut_playback_speed_step' to 'shortcuts_volume_step', 'shortcuts_playback_speed_step' to not mix those keys with real Shortcuts.
shortcut_ = shortcut;
shortcuts_ = helper variable;

replace defined storage with proper names
…aybackSpeed

renamed 'shortcut_volume_step', 'shortcut_playback_speed_step' to 'shortcuts_volume_step', 'shortcuts_playback_speed_step' to not mix variable keys with function Shortcuts.
@ImprovedTube
Copy link
Member

ImprovedTube commented Jun 30, 2024

Problems:

"My active features" cant distinguish between default YT shortcut or User override using same key combination.

#2425

but no, im not taking over shortcuts!

😅 Please just think about it some more 😅
I think much of it happened at brain 😌

rename 'shortcut_volume_step', 'shortcut_playback_speed_step' to 'shortcuts_volume_step', 'shortcuts_playback_speed_step' to not mix variable keys with function Shortcuts. shortcut_ = shortcut; shortcuts_ = helper variable;

maybe speed_step & volume_step? People want clickable buttons for speed too, which also has a complicated name (playerForcedPlaybackSpeed) Speed could have 4 sub-options and more sub-sub-options speed[buttons;keys;step-size;permanent[exceptions;requirements] ]

@ImprovedTube
Copy link
Member

ImprovedTube commented Jun 30, 2024

first issue #2042 (comment) it does absolutely take over keyboard and mouse events preventing Play, im pretty sure old code also did it.

( what line/s? )
( autoplayDisable() can also require that the page was loaded or navigated or activated/viewed within the last few seconds. )

@raszpl
Copy link
Contributor Author

raszpl commented Jul 1, 2024

first issue #2042 (comment) it does absolutely take over keyboard and mouse events preventing Play, im pretty sure old code also did it.

( what line/s?

window.addEventListener('keydown', function (event) {...}, true); that true means it is capturing = eats all key presses if it decides to preventDefault()

but reading further it doesnt prevent other listeners on same element https://www.aleksandrhovhannisyan.com/blog/interactive-guide-to-javascript-events/#stoppropagation-vs-stopimmediatepropagation so false alarm :)

) ( autoplayDisable() can also require that the page wasn't loaded or navigated or activated/viewed more than a few seconds ago. )

I think we already talked about this. Someone loads page in the background tab = YT will autoplay only after switching to that tab, user switches to the tab after few minutes, settimeout would make that autoplay pass.
Every time you want to use settimeout just start with the assumption its a bad idea :-]

@ImprovedTube
Copy link
Member

#2431

@ImprovedTube
Copy link
Member

acc789c

ImprovedTube.playerRotateButton = function () {
if (this.storage.player_rotate_button === true) {
var svg = document.createElementNS('http://www.w3.org/2000/svg', 'svg'),
path = document.createElementNS('http://www.w3.org/2000/svg', 'path');
svg.setAttributeNS(null, 'viewBox', '0 0 24 24');
path.setAttributeNS(null, 'd', 'M15.55 5.55L11 1v3.07a8 8 0 0 0 0 15.86v-2.02a6 6 0 0 1 0-11.82V10l4.55-4.45zM19.93 11a7.9 7.9 0 0 0-1.62-3.89l-1.42 1.42c.54.75.88 1.6 1.02 2.47h2.02zM13 17.9v2.02a7.92 7.92 0 0 0 3.9-1.61l-1.44-1.44c-.75.54-1.59.89-2.46 1.03zm3.89-2.42l1.42 1.41A7.9 7.9 0 0 0 19.93 13h-2.02a5.9 5.9 0 0 1-1.02 2.48z');
svg.appendChild(path);
this.createPlayerButton({
id: 'it-rotate-button',
child: svg,
opacity: 0.85,
onclick: function () {
var player = ImprovedTube.elements.player,
video = ImprovedTube.elements.video,
rotate = Number(document.body.dataset.itRotate) || 0,
transform = '';
rotate += 90;
if (rotate === 360) {
rotate = 0;
}
document.body.dataset.itRotate = rotate;
transform += 'rotate(' + rotate + 'deg)';
if (rotate == 90 || rotate == 270) {
var is_vertical_video = video.videoHeight > video.videoWidth;
transform += ' scale(' + (is_vertical_video ? player.clientWidth : player.clientHeight) / (is_vertical_video ? player.clientHeight : player.clientWidth) + ')';
}
if (!ImprovedTube.elements.buttons['it-rotate-styles']) {
var style = document.createElement('style');
ImprovedTube.elements.buttons['it-rotate-styles'] = style;
document.body.appendChild(style);
}
ImprovedTube.elements.buttons['it-rotate-styles'].textContent = 'video{transform:' + transform + '}';
},
title: 'Rotate'
});
}
};

@raszpl
Copy link
Contributor Author

raszpl commented Jul 8, 2024

c00004c
decrese

:-]

shortcutIncreasePlaybackSpeed shortcutDecreasePlaybackSpeed
keeping familar speed stepping;

My rewrote function respects stepping is set by shortcuts_volume_step
The one you reverted to doesnt respect that setting and only increments by 0.05

max.;

the one you reverted to doesnt respect any limits and simply crashes after reaching 16, at this point shortcuts stop working altogether until page reload

decimal

What do you mean?

shortcuts.js doesnt even load now because #2446 (comment) "braces, indentation, blockly, etc. are all the same."
they arent the same :) They are helpers aiding in writing clean code that works.

"Uncaught SyntaxError: Unexpected token 'var' (at VM481 shortcuts.js:358:2)" :(
Linter:

/home/runner/work/youtube/youtube/js&css/web-accessible/www.youtube.com/shortcuts.js
 358:2  error  Parsing error: Unexpected keyword 'var'

I didnt rewrite those functions to make it look the way I like it, I rewrote it because it was broken :(

e2c24bf
there was no type there :

shortcuts_volume_step: {

be504b7

			chrome.storage.local.set({below_player_pip: false})
		}
			chrome.storage.local.set({below_player_pip: false})}

this isnt one liner, this is broken code leading to bugs

Ok, I give up. Ill return in a month and see if anything changed. :(

ImprovedTube added a commit that referenced this pull request Jul 8, 2024
ImprovedTube added a commit that referenced this pull request Jul 8, 2024
@ImprovedTube
Copy link
Member

Ok, I give up. Ill return in a month and see if anything changed. :(

Please don't get exhausted 🥹♡ @raszpl

there was no type there

it changed from volume_step to shortcuts_volume_step (not from shortcut_volume_step)

My rewrote function respects stepping is set by shortcuts_volume_step
The one you reverted to doesnt respect that setting and only increments by 0.05

reverted speed only.

one might just has to chose a temporary revert /quick-fix within a given day or hour in favor of today's and tomorrow's users.
(old news)

this isnt one liner

Want automatic linting
/manually back and forth defeats some of the purpose.

They are helpers aiding in writing clean code that works.

Yes, the old point is just: Any such format/s can co-exist with intentional other structure beyond it, like density, meant to suggest not spending time there. We are currently more relevant for this project than linting.

"braces, indentation, blockly, etc. are all the same."
they arent the same :)

They are different themes for the same scheme (another one is flowcharts). Variants could be switched dynamically just like a CSS files.

ImprovedTube added a commit that referenced this pull request Jul 8, 2024
@ImprovedTube
Copy link
Member

the one you reverted to doesnt respect any limits and simply crashes after reaching 16, at this point shortcuts stop working altogether until page reload

Chrome's limit comes with no crash & Firefox has no limit

The one you reverted to doesnt respect that setting and only increments by 0.05

( The names changed )

@ImprovedTube
Copy link
Member

ImprovedTube commented Jul 9, 2024

...PS: i also did regret a lot already in June, that i hardly worked here for several weeks, nor brought the time to say much unique or interesting or well-enough-written. ( And not everything i note down (quickly, in your PRs or anywhere) is to bother you, and could / should be moved to another list. Etc. )

Since i was a child I looked forward to motivating/meaningful teamwork, and still hope to enjoy/achieve better/more of that.
( While i can't really imagine us disagreeing about automatic workflows or commits. While we are writing a lot of a volunteer history. And a lot of relevant Open-Source is shockingly under-staffed (the ideas that didn't even start potentially more than those that started).
On a long term, next to reaching ever more depth in certain skills, do you think of choosing your overall impact/helpfulness wisely or anything more specific? @raszpl

For this project i should / we could prioritize the tasks very well (https://github.com/orgs/code-charity/projects/1 or better) And we should have [had an] assistant/intern/s already (it should be easy funding that with many users in a connected world with extremely imbalanced/unfair economy. ( And by now this project can assumably be accepted for GSoC or so #2043. )

ImprovedTube added a commit that referenced this pull request Jul 28, 2024
@ImprovedTube
Copy link
Member

hi! @raszpl, just saw your fork! (, so that your work currently has no users, because i reviewed automated whitespace changes, while you coincidentally saw the moment, that i undid some before recreating those, that do not lose human-written content) (where to report issues in the current state? /why disable the issues tab?)

Isn't it wonderful (mentally & and for the users), to have a project where at least 2 people have thought about 100s or 1000s of issues? - Compared to most teams our work was very efficient and can be more efficient ( _given that i use to say that most comments are optional & assuming you agree. And that i said, i regret that i had to be mostly / mentally inactive in June unfortunately the misunderstandings where you seem to "have to explain too much" (which might helps the project if more people still read that later, while for me the keywords /links are usually enough.)
While, I might try to express my respect for your achievements, I'm not a paradox nuisance for this project and your productivity.


To finish to recur the old topic of whitespace: i look forward to make sweeter use of longterm development processes, like the "blame" column on github:
git blame
(Look at this sad mess. (Unfortunately this doesnt have the "ignore white space button", said such before 😁 following #2073?. ) & in #2391 you sounded understanding, "loss of git history")

More importantly. Each of our effort is worth more!
and I didn't want to frustrate motivated contributors like @unnamed-orbert (with crowdin) or @4yman-0
crowdin

The project is still effective, even if i happen to do some boring/annoying commit-history-rewriting later,
(We might soon clean up 1000+ commits from the history to invest in a tidy future! (Or might do something "more urgent" first. Choice is ours.) (While lossless linting & whitespace, can be effortlessly allowing no useless variance (like spaces instead of tabs) in the whole history, so that you don't have to fix it later. So that will be nicer and automated (and git or a convention is not an end in itself.)

@raszpl
Copy link
Contributor Author

raszpl commented Aug 3, 2024

hi! @raszpl, just saw your fork! (, so that your work currently has no users,

it has the only user that counts - Me :)

why disable the issues tab?

This is my private repo. I left it visible so you can copy anything you want from it no questions asked.

@ImprovedTube
Copy link
Member

I left it visible so you can copy anything you want from it!

Yay!! @raszpl then you (or i?) should make it a PR ( except the deletions)

it has the only user that counts - Me :)

Our work for all the others can have significantly larger effects (user satisfaction), and more indirect ones if we try one way or another sooner than later. (Such as signing up at open-collective, to donate against hunger or pay some work, etc.)
( (as we know,) your achievement to have read a lot of the code makes you a potential architect of the project. (More than other contributors and more than for a project new to you)

(

any agpl3 code shipped means whole project must be agpl-3

raszpl/tweaks4ytube#32

Not true here (as elaborated) (and if it was that would be a bad thing.)
)

@ImprovedTube
Copy link
Member

...So we should have a "merge-/sync-day" then asap🎆(?) @raszpl
(Do you envision to dedicate any hours to that? the less the higher the ratio of mine i suppose👍 /the fewer comments)

@ImprovedTube ImprovedTube self-assigned this Aug 10, 2024
@ImprovedTube
Copy link
Member

private ... visible so you can copy

...then you want to remove or fix the license document?
(please confirm so that i start @raszpl)

nice DOM statistics btw

@ImprovedTube
Copy link
Member

💭💭
hi @raszpl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: can set the same shortcut as YouTube (=twice). Please Review our shortcuts.js vs. youtube defaults.
2 participants