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

comparing the new-unfinished & old version (any missing code to migrate?) #6

Open
ImprovedTube opened this issue Feb 15, 2023 · 19 comments

Comments

@ImprovedTube
Copy link
Member

ImprovedTube commented Feb 15, 2023

1. Sep-2021 : 105kb JS + 22kb CSS

2. Now it is more CSS, less JS. Yet unfinished. This goes in hand with all known bugs:

( what we use live is from ImprovedTube 3.945 August 2022) - None of the bugs appeared before.
(+ 2.1. just removed an old color-picker code, without reviewing changes: f416701 )

8 bugs introduced in the live version: #3

history manager has a version from May 2021 with the mark up working! ( & <textarea> ?)


More:

satus July18 from ImprovedTube Beta 3.1001.zip

  • possible this has a relevant/most recent edit, if we compare

( Inbetween, still almost the same as current / 2023):

( 2. Jan-2022 87kb JS + 37kb CSS Jan2022.zip )


Finally: [oct2022 but again MINIFIED unfortunately.zip] (https://github.com/code-for-charity/SATUS/files/11045240/oct2022.but.MINIFIED.zip) - possibly could also have any good edit, but yet we need an automated way to compared, despite minified. how?

@ImprovedTube ImprovedTube changed the title compare new-unfinished & old version (any missing code to migrate?) comparing the new-unfinished & old version (any missing code to migrate?) Mar 22, 2023
ImprovedTube added a commit that referenced this issue Mar 22, 2023
@raszpl
Copy link

raszpl commented May 2, 2023

holy 100KB batman
some of the stuff in satus is redundant, like https://github.com/code-charity/youtube/blob/73cbd813f4db2b283343e23ef664ae07fff6afe4/js%26css/satus.js#L2879
https://github.com/code-charity/youtube/blob/73cbd813f4db2b283343e23ef664ae07fff6afe4/js%26css/satus.js#L2861
https://github.com/code-charity/youtube/blob/73cbd813f4db2b283343e23ef664ae07fff6afe4/js%26css/satus.js#L3056
https://github.com/code-charity/youtube/blob/73cbd813f4db2b283343e23ef664ae07fff6afe4/js%26css/satus.js#L2870
https://github.com/search?q=repo%3Acode-charity%2Fyoutube+camelize+&type=code
why even camelize anything?

or unneeded https://github.com/code-charity/youtube/blob/73cbd813f4db2b283343e23ef664ae07fff6afe4/js%26css/satus.js#L2861
https://github.com/code-charity/youtube/blob/73cbd813f4db2b283343e23ef664ae07fff6afe4/js%26css/satus.js#LL2926C22-L2926C22
https://github.com/code-charity/youtube/blob/73cbd813f4db2b283343e23ef664ae07fff6afe4/js%26css/satus.js#L2907
https://github.com/code-charity/youtube/blob/73cbd813f4db2b283343e23ef664ae07fff6afe4/js%26css/satus.js#LL3086C22-L3086C22
https://github.com/code-charity/youtube/blob/73cbd813f4db2b283343e23ef664ae07fff6afe4/js%26css/satus.js#LL3065C21-L3065C21

etc etc

missing mime types YT is specifically testing for when loading player https://github.com/code-charity/youtube/blob/73cbd813f4db2b283343e23ef664ae07fff6afe4/js%26css/satus.js#L2966
AV1 "video/mp4; codecs=av01.0.05M.08"
H.264 "video/mp4; codecs="avc1.640028"; framerate=30"

How to decode H.264 name from numbers: Profile Level
Constrained Baseline 2.1 avc1.42c015
Constrained Baseline 3.0 avc1.42c01e
Main 3.0 avc1.4d401e
Main 3.1 avc1.4d401f
High 3.0 avc1.64001e
High 3.1 avc1.64001f
High 3.2 avc1.640020
High 4.0 avc1.640028

audio https://github.com/code-charity/youtube/blob/73cbd813f4db2b283343e23ef664ae07fff6afe4/js%26css/satus.js#L2939
AAC low complexity (AAC-LC) "audio/mp4; codecs="mp4a.40.2""
opus "audio/webm; codecs="opus""
"audio/webm; codecs="opus"; channels=2"
"audio/webm; codecs="opus"; channels=99"

a lot of fat to trim and stuff to update. After reading the code I dont see ability to manipulate settings by other settings :( So its impossible to program one button to automatically switch another option off :(. Ill look into how easy it would be to add this functionality (maybe needed for overriding codecs).

@raszpl
Copy link

raszpl commented May 10, 2023

Two days of staring at this code and its starting to make sense :) Its actually quite logical and pleasant to work with.

I dont see ability to manipulate settings by other settings

I found an indirect way by sending events, calling functions directly or directly manipulating .dataset.value/.storage.value

I also found few bugs:

  1. https://github.com/code-charity/youtube/blob/7e56c0c0005240da1bc2f9b5aa2a31e527410eeb/js%26css/satus.js#L1177
            this.modalProvider.dispatchEvent(new CustomEvent('cancel'));
            this.modalProvider.close();

this doesnt seem to work, looks like despite settimeout element is removed() before Event has a chance to reach it. I already have a patch for this almost ready.

  1. Too many verbose click listeners. Modal dialog (those with ok/cancel) has 3 simultaneous ones with two bubbling events per click screwing things up. Im testing the fix.

  2. https://github.com/code-charity/youtube/blob/7e56c0c0005240da1bc2f9b5aa2a31e527410eeb/js%26css/satus.js#L491

satus.events.trigger = function(type, data) {
  var handlers = this.data[type];

  if (handlers) {
    for (var i = 0, l = handlers.length; i < l; i++) {
    try {handlers[i](data);} 
	   catch(err){console.log(err);}
    }
  }
};

the reason for try/catch here is a bug where this function is being called with type = storage-set and data=unknown here https://github.com/code-charity/youtube/blob/7e56c0c0005240da1bc2f9b5aa2a31e527410eeb/js%26css/satus.js#LL1004C1-L1004C45
try {satus.events.trigger('storage-set');}
and this.data actually having bogus undefined storage-set element. Fix pending.
EDIT: Ha, found it https://github.com/code-charity/youtube/blob/94bd68583408e5b20dba0315a9edefeb168723d2/menu/index.js#L49
satus.events.on('storage-set', extension.attributes);
this is never used anywhere and causes this problem together with satus.events.trigger('storage-set'); I think maybe someone was in the process of implementing new feature and didnt finish?

  1. https://github.com/code-charity/youtube/blob/7e56c0c0005240da1bc2f9b5aa2a31e527410eeb/js%26css/satus.js#L968 satus.storage.set overwrites whole config (chrome.storage.local.set) even if only one option is being changed. Not really a bug, but a weird undesirable action.

@raszpl
Copy link

raszpl commented May 11, 2023

bug 3 fix code-charity/youtube#1676 Still no idea why this functionality is there (why call extension.attributes() every time some option is changed?) :) but at least now its working as it was intended.

@ImprovedTube
Copy link
Member Author

ImprovedTube commented May 14, 2023

dont see ability to manipulate settings by other settings :(

check (yes, only this:
code-charity/youtube@76c91d4 )

@ImprovedTube
Copy link
Member Author

ImprovedTube commented May 14, 2023

why camelize

just answered in #1634

@ImprovedTube
Copy link
Member Author

I think maybe someone was in the process of implementing new feature and didnt finish?

mostlikely, includes some unfinished plans worth noticing.
Dont know if/when the architect will be back, all we have is public.
Please make comments in code.
Your effort can be our documentation. (Somebody can help also turn it into wiki pages etc. as said.)

@raszpl
Copy link

raszpl commented May 14, 2023

why camelize

just answered in #1634

If you really want smaller js files (meaningless in case of menu for an extension) you can always minify the release package. Mixing naming conventions just makes code less readable. It adds noise for no gain whatsoever. This code doesnt live in a hot loop of some performance sensitive function, its extension menu - gets executed only when user wants to change options.

dont see ability to manipulate settings by other settings :(

check code-charity/youtube@76c91d4

1 "need 1ms since JS executes faster than firefox storage"

Patch comment is not entirely accurate, settimeout is not a proper fix for race conditions. What is happening here is using async storage function (chrome.storage.local.set in onclick event) and expecting it to take immediate action (satus.storage.get('transcript')). settimeout will help in that case, but not the way one might expect. Its not that you need to wait some magic number of milliseconds for slow storage, its emptying event loop so async functions can complete
What the heck is the event loop anyway? | Philip Roberts | JSConf EU https://www.youtube.com/watch?v=8aGhZQkoFbQ

The problem here is how Satus gui library handles clicking on elements in separate event. Here Appearance/Sidebar/Transcript element has two onclick handlers simultaneously (what I mentioned earlier as "2. Too many verbose click listeners")
One is https://github.com/code-charity/youtube/blob/76c91d43875737de1e2f6802b102b3ee681d7ff5/menu/skeleton-parts/appearance.js#L814
and the other https://github.com/code-charity/youtube/blob/76c91d43875737de1e2f6802b102b3ee681d7ff5/js%26css/satus.js#L2476

Im thinking on ways to rework whole thing into something less convoluted.

2 nextSibling.click() is fragile code. Its meant to target "To the side! (No page margin)" and will break next time someone decides to move options around. Better to call by name.

@ImprovedTube
Copy link
Member Author

ImprovedTube commented May 14, 2023

a lot of fat to trim

yes, besides it could be an unlimited ordered collection, when a build script could generally include removal of unused functions.

One original thought/reason for Satus was to keep different extension features & GUI maintainable efficiently.
& keep moving code from all extensions to Satus. ( + then could add a template extension here with everything)
For example it makes no sense that Frame by frame & Looper are standalone extensions, gotta copy them into ImprovedTube too with (optional) global permission.

Thank you again @raszpl. Looking forward / excited. You probably already solved something else pending since #1439 🤩

@ImprovedTube
Copy link
Member Author

ImprovedTube commented May 14, 2023

If you really want smaller js files

sorry, the cause was human eyes, not cpu. See here. (The idea there was just to make collaborated code as short as possible. To reduce our (contributor's) time it takes to read it. Increasing cognitive efficency, decreasing overhead. (Underscores are considered overhead by a majority of JS developers. (Yet not for translators maybe. Thats why one could maybe reason to have both. But we dont need to )

Patch comment is not entirely accurate,

👍 good info. (just a hotfix & the intentional UX advantage (250ms delay) could rather be animated.)

Im thinking on ways to rework whole thing into something less convoluted.

🥳🎉 *hyperventilates*

2 nextSibling.click() is fragile code. Its meant to target "To the side! (No page margin)" and will break next time someone decides to move options around. Better to call by name.

👍 just to make sure you saw the current way this before possibly implementing it well. (Sometimes instead of "check" i should say "btw, just incase...)

@ImprovedTube
Copy link
Member Author

"shorter code for human readers"

menu

(we agree)

the following is bloated to read&remember
(3 different names) (in case of human editors & not generating the file somehow)

autopause_when_switching_tabs: {
 component: 'switch',
 text: 'autopauseWhenSwitchingTabs',
 storage: 'player_autopause_when_switching_tabs'

Same information, just shortest & logically ordered
switch:'PauseWhileInAnotherTab',

Optionally another 1 character shorter&tidier for humans (if adhering/phrasing a html UI-templating-Syntax )
<Switch PauseWhileInAnotherTab>
?
( especially incase of:
<Switch PauseWhileInAnotherTab disabledBy="PauseWhileA2ndPlayerStarts">
Etc... )

(- And whle considering that we can assess/define why&how Satus might potentially be used combined with the predominant JS libs
..)

@raszpl
Copy link

raszpl commented May 15, 2023

(Underscores are considered overhead by a majority of JS developers.)
answer based on google trends!?!?!

Everything is overhead when performance is considered, but here we shouldnt care about absolute optimal code speed because this is not a critical function of this code. This code already works same way on 1GHz Atom laptop as it does on 4GHz desktop to human perception.
No need for this obsession with removing characters from function names while at the same time you ship 0.5MB of fonts :) (which are absolutely fine btw).

the following is bloated to read&remember

Function names and labels arent bloat, there is no point making them smaller for the sake of making them smaller. On the other hand using two naming conventions at the same time is iffy. Finally mixing two naming conventions is bad
https://github.com/code-charity/youtube/blob/ee0de83adb8bc5b5385c9e0ae298a1595648575f/menu/skeleton-parts/appearance.js#L867
compactSpacing
satus.storage.data.compactSpacing
Now we cant just fix rename the storage variable and forget about it, people using this option already had it stored under compactSpacing :) fix for section name (includes reformatting) code-charity/youtube#1679

autopause_when_switching_tabs: {
 component: 'switch',
 text: 'autopauseWhenSwitchingTabs',
 storage: 'player_autopause_when_switching_tabs'

Same information, just shortest & logically ordered
switch:'PauseWhileInAnotherTab',

  1. autopause_when_switching_tabs is GUI section name

  2. text: 'autopauseWhenSwitchingTabs' is localization text name. This could be optimized by renaming autopauseWhenSwitchingTabs to autopause_when_switching_tabs in all locale files, and then we can delete
    text: 'autopauseWhenSwitchingTabs'
    line.

  3. storage: 'player_autopause_when_switching_tabs' finally this is needed because of backward compatibility, Im guessing autopause_when_switching_tabs used to be called player_autopause_when_switching_tabs :]. Hell, we could rewind back time and just go back to

player_autopause_when_switching_tabs: {
    component: 'switch',

with GUI section name matching storage variable name and being used directly as lookup to locale.json. This might require some code changes to https://github.com/code-charity/youtube/blob/ee0de83adb8bc5b5385c9e0ae298a1595648575f/js%26css/satus.js#LL802C12-L802C12

here is one example where someone renamed GUI section without reason
https://github.com/code-charity/youtube/blob/ee0de83adb8bc5b5385c9e0ae298a1595648575f/menu/skeleton-parts/appearance.js#L215
all this change did was the need to add another line
storage: "player_hide_controls",


btw what are tags ?
https://github.com/code-charity/youtube/blob/ee0de83adb8bc5b5385c9e0ae298a1595648575f/menu/skeleton-parts/appearance.js#L610
this looks like text that should display in a tooltip when user hovers mouse over option, but its not implemented anywhere.

@ImprovedTube
Copy link
Member Author

ImprovedTube commented May 15, 2023

Hi @raszpl, tags are for search / synonyms #748
1.1.

@ImprovedTube
Copy link
Member Author

ImprovedTube commented May 16, 2023

cpu

Sorry, while the thread #1211 about codec & dom is pinned, #1634 wasn't meant about computers, only humans.

no point

Humans count/quantify work. If a project will be X% shorter, without decreasing readbility, then editors will be Y% faster & happier. Even 1% matters with many users (+ planning for years, welcoming contributors, ...)

camelCase

No opinion, didnt start this in our code nor the world!
Maybe rather read the idea /concept of deleting line breaks for human readers in permanent/immutable code. (Might be more inspiring - that's just in case you'd like any of #1634 😊 otherwise can ignore that issue-thread)

camelCase

just has the point of being distiguishable (nearly equally?) aBC vs. a_b_c , while understore is bigger to read (& type)

  • Names make sense often to look like one unit (?) (~no spaces) (while underscore looks different visually more like a.b )

google trends ?!?

The mentioned point was, the average developer on github. (+the global trend, as around 43% of code on github seems to use camelCase but it is increasing since 2004 in global search interest too. We can repeat & filter GitHub by recent years, so that we assumably see camelCase as the bigger half in all recent code(>50%). (While in javascript it is around 94% allover already. Which is ~30% of all code and also more considering recent years & NPMs )

3 names

Consider the shortest versions i mentioned - Only these chars are required, when to be written by humans efficiently (+ most of all GUI templates /html start with mentioning GUI element types before their labels=values)

(Not saying we should bother before important bugs 😅 & more urgent /interesting things. Just noting down this topic at all, here in this repo. )

**2. Currently we only have names with underscore for values in GUI, this also appear when there is no translation. Since Users might be less used to camelCase. - i also thought of removing that /keeping one naming convention only. (We can always improve translation & locales later still. While our project might outgrow the rigid extension locales structure anytime like this:
code-charity/youtube#1539 (comment)

backwards compatibility

(Can rename old storage names @update-installation in background.js)

@raszpl
Copy link

raszpl commented May 17, 2023

Humans count/quantify work. If a project will be X% shorter, without decreasing readbility, then editors will be Y% faster & happier.

Do you have any links/references to literature on this subject? Im not familiar with the concept. Iv been taught to make code readable and fast, not smaller by reducing size of variables.


Why is checkbox/switch .dataset.value a string ('true') while .storage.value a boolean? both represent same data and both should be boolean.


What is the relationship between

  1. Auto-pause while I'm not in the tab
  2. Pause while I watch a 2nd video
  3. autoPip

I see enabling 1 will disable 2, enabling 2 will disable 1, but enabling 3 will disable 1 and enable 2?? I dont understand :)
I most likely got the logic wrong in code-charity/youtube#1681, but I reworked it so in no longer relies on previousSibling/nextSibling, now even moving options around wont break, and its clearer what is being changed by name. It should be trivial to set autoPip up in the way its supposed to interact.


optimize_codec_for_hardware_acceleration needs a list of GPUs with hardware video acceleration capabilities.

@ImprovedTube
Copy link
Member Author

ImprovedTube commented May 17, 2023

consistency

(As of now our code is *mostly using camelCase which is popular among [JS-]developers)
(except for the GUI values, which might appear to users - if not yet translated)

"Minification for Humans"

Just a big topic yet (should be a discussion-thread, not an issue)
Deleting line breaks reduces readability (until horizontalScrolling - pageLeft & pageRight are established)
Besides * correctly predicting * what lines won't ever be edited will mean less scroll_height for free. Thus more readable, because a lot of scrollHeight is lenghty to read, not efficient, not fun, less readable,.

fast

If #1211 is done well, we might save the average user 3 minutes + $0.2 in electricity per year. just relevant on scale (+ the project is growing). Might also be able to save them some seconds per year if we werent bound to JS.

reference ... readable ... not small

mmm 🤔 - small & readable inevitably correlate. Together they are: "concise"
( Code should be editable_expandable_enhancable, no? )
( While what should be pretty readable(+prouncable) is: Song lyrics, just like stories for kids & poems.)

"editable is the new readable" (- this curriculum literature doesn't seem to be written yet (zero search results!!) - we'll have to start writing it.) ( justification for our PhD-topic: https://trends.google.com/trends/explore?date=all&q=editable,readable,expandable,enhanceable )

White spaces are "cheap readablity" - at the expense of time.
One word per line is always very_readable
(at the expense of taking several times longer to read/scroll the whole & probably even losing context.) -
(Longer_but_not_easier takes more time = Less_readable )
(Shorter_but_not_harder takes less time = More_readable )
Thus more_readable code is a bit of an oxymoron / paradox.
Because more_readable usually also means less_readable.
"More_readable" is convicing if it it takes every reader less time to read.
Else it might be questionably overdone - or personal preference/taste.
If it is taste, then we might want to chose personally, since possibly our style and thoughs might include more personal things (there might be types of programmers)
or else, as a public project we could adjust to the most common way, or the growing one, which might be for a reason.

One more test: According to this camelCase makes ~12/13 in JS code
the case of camelCase
the case of camelCase

& 4/5 overall:
the case of camelCase
the case of camelCase

Also, kinda predictably we appear first here:
the case of camelCase
(where camelCase even is >82% overall:)
the case of camelCase

@ImprovedTube
Copy link
Member Author

ImprovedTube commented May 17, 2023

Why is checkbox/switch .dataset.value a string ('true') while .storage.value a boolean? both represent same data and both should be boolean.

Dont know!!

BTW here is a slightly related Brainstorming: code-charity/youtube#1685


.. also since you analysed shortcuts issues earlier! :D ( code-charity/youtube#1565 - just sharing my latest thought-process for the project - hopefully not flooding you, but just adding some interesting /innovative perspective. extra motivation. Hopefully we can share some work once /based on special skills/experience (mine isn't JS 🤫😆just yet).

...


optimize_codec_for_hardware_acceleration needs a list of GPUs with hardware video acceleration capabilities.

Yay, let's go


What is the relationship between

Auto-pause while I'm not in the tab
Pause while I watch a 2nd video
autoPip
I see enabling 1 will disable 2, enabling 2 will disable 1, but enabling 3 will disable 1 and enable 2?? I dont understand :)
I most likely got the logic wrong in code-charity/youtube#1681, but I reworked it so in no longer relies on previousSibling/nextSibling, now even moving options around wont break, and its clearer what is being changed by name. It should be trivial to set autoPip up in the way its supposed to interact.

  • (1) Auto Pause, should auto pause & unpause when leaving a tab & coming back (includes all cases of 2. :

  • (3) autoPip isnt compatible with 1. because the PictureInPicture play would be autopaused because it is considered a tab. Yet then it is consequent to keep 2. instead

@raszpl
Copy link

raszpl commented May 17, 2023

Seems consistent.
(As of now our code is using camelCase which is popular among [JS-]developers)
(except for the GUI values, which might appear to users - if not yet translated)

so far my favorite
https://github.com/code-charity/youtube/blob/9bc78806a04e2b4debf8ce46ce5c9d58db6d6aef/menu/skeleton-parts/active-features.js#L36
parent_object.parentObject && !parent_object.parentObject.category
:o despite mixed cases its quite readable code :)

If #1211 is done well, we might save the average user 3 minutes + $0.2 in electricity per year. just relevant on scale (+ the project is growing). Might also be able to save them some seconds per year if we werent bound to JS.

1 is very ambitious, difficult and fragile. YT changes the page often. I also really dislike convoluted heavy YT dynamic DOM with all those moving parts and everything done with javascript. Layout from before the 2020 change was mostly html, now everything is client side and slow :((((

2 is ready, needs a list of GPUs with supported codecs.

Why is checkbox/switch .dataset.value a string ('true') while .storage.value a boolean? both represent same data and both should be boolean.

Dont know!!

great :) so I can change it after verifying everything works?

3. autoPip isnt compatible with 1. because the PictureInPicture play would be autopaused because it is considered a tab. Yet then it is consequent to keep 2. instead

so autoPip should disable 1 and force enable 2?

@ImprovedTube
Copy link
Member Author

ImprovedTube commented May 17, 2023

hi, are you on Discord? ( https://discord.gg/aHpjrhf )

is ready, needs a list of GPUs with supported codecs.

👍👍

parent_object.parentObject && !parent_object.parentObject.category

😅

DOM ... ambitious

yes, (you might saw 😳 hackademix/noscript#291) (might start with Firefox & offering mobile youtube on desktop. (Also always wanted to bring the desktop sibling of "show desktop site"(mobile browser) as a single-click standalone extension "show mobile site" (no popup menu, just a checkup as extension icon, remembering per sub domain))

... autoPiP ...

code-charity/youtube@8bd2be2

@ImprovedTube
Copy link
Member Author

  1. code-charity/youtube@7e56c0c/js%26css/satus.js#L968 satus.storage.set overwrites whole config (chrome.storage.local.set) even if only one option is being changed. Not really a bug, but a weird undesirable action.

👍♡

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

No branches or pull requests

2 participants