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

Patch 1 #1681

Merged
merged 6 commits into from
May 17, 2023
Merged

Patch 1 #1681

merged 6 commits into from
May 17, 2023

Conversation

raszpl
Copy link
Contributor

@raszpl raszpl commented May 17, 2023

ModalHelper no longer needed after fixing up modal.confirm default ok/cancel dialog
Added new switch 'custom' property. Disables default click handler, lets us display modal dialog without switching underlying option, cancelling modal dialog means underlying switch is never touched.
adding switch.flip(), lets us manually change switch position
formatting, spaces to tabs
player_quality reflects 1080p resolution limit when player_h264 is enabled
use id to directly switch linked options instead of relying on previousSibling/nextSibling

ModalHelper no longer needed after fixing up modal.confirm default ok/cancel dialog
Added new switch 'custom' property. Disables default click handler, lets us display modal dialog without switching underlying option, cancelling modal dialog means underlying switch is never touched.
adding switch.flip(), lets us manually change switch position
formatting, spaces to tabs
player_quality reflects 1080p resolution limit when player_h264 is enabled
use id to directly switch linked options instead of relying on  previousSibling/nextSibling
moved repeating code in player_codecs into common function
player_quality now clearly shows Video wont work when codecs are disabled.
@ImprovedTube
Copy link
Member

@raszpl thank you again! time for a new stable release? 😎

@ImprovedTube ImprovedTube merged commit 417ae81 into code-charity:master May 17, 2023
@raszpl
Copy link
Contributor Author

raszpl commented May 17, 2023

some testing before would be nice, like please verify that the
Auto-pause while I'm not in the tab
Pause while I watch a 2nd video
autoPip
are still doing what they are supposed to

@raszpl raszpl deleted the patch-1 branch May 17, 2023 17:49
@ImprovedTube
Copy link
Member

ImprovedTube commented May 17, 2023

thanks! @raszpl

verify that

fixed that.

All your work is now pending in all stores.

A bug is: After installation, h.264 needs to be clicked twice before it reacts and the modal appears

@raszpl
Copy link
Contributor Author

raszpl commented May 18, 2023

A bug is: After installation, h.264 needs to be clicked twice before it reacts and the modal appears

and its because of code-charity/SATUS#6 (comment)

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

Now I know why it was a string :] https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dataset it cant be anything else.
edit: because of this a few more options are currently broken, will fix tomorrow

New plan. Instead of storing 'false' we can just "delete this.dataset.value", then it works just like boolean false.

Am I correct in assuming

                autoplay: {
                        component: 'switch',
                        text: 'autoplay',
                        value: true

"value: true" here means this option starts its life enabled until user changes it? then why are those seven set to "value: false"
https://github.com/search?q=repo%3Acode-charity%2Fyoutube+%22value%3A+false%22&type=code
isnt "value: false" the default behavior when no value is defined? thus those 7 entries are not needed?

oh and btw good job testing with empty config!

@raszpl
Copy link
Contributor Author

raszpl commented May 18, 2023

solidjs/solid#1101 and things start to get clear, using html attributes for boolean values is difficult and everyone is forced to make special cases for it.
Now I know why there are "value: false" in appearance.js in the first place - someone more clever than me knew this could be a potential problem and put it here
https://github.com/code-charity/youtube/blob/c84f30d341989d670aee7662321c92fcefb92a55/menu/skeleton-parts/appearance.js#LL756C1-L760C45
to be able to do if (this.dataset.value === 'false') { instead of (!satus.storage.get('transcript')) if needed.

html attributes as booleans are cumbersome :/

Two ways going about it, we either:

  1. use explicit "value: false" to initialize this.dataset.value to "false", without it

    if (this.dataset.value === 'false') {

    if (this.dataset.value === 'false') {
    will not trigger because initially value is not defined at all, so we are forced into

    and set the option to 'false', thus needing to click twice for the option to start working :)

  2. change whole mechanism for storing .switch dataset values to ""| undefined eliminating the need for if (this.dataset.value === 'false') { and letting us do if (!this.dataset.value) {. Thats what I meant by

Instead of storing 'false' we can just "delete this.dataset.value"

Ill just do the first for now as its easiest :)

@raszpl raszpl mentioned this pull request May 18, 2023
@ImprovedTube
Copy link
Member

ImprovedTube commented May 20, 2023

isnt "value: false" the default behavior when no value is defined? thus those 7 entries are not needed?

Looks like it can be removed & isnt by the author of satus; (Seems @dodieboy first used it https://github.com/code-charity/youtube/pull/1020/files#diff-3df6958c77d615b266b54fcf12fadf313e7c1e4b168f0c61ea8c3424eb39eb8c )

oh and btw good job testing with empty config!

While people have config files, as an app, I only consider the repo files, unless a human sets me up for auto-testing 🙊

initially value is not defined

exactly😊 ( #1685 )
if (ImprovedTube.storage.player_60fps === false)
(or to avoid js errors browser consoles logging undefined variables: (ImprovedTube.storage.player_60fps && ImprovedTube.storage.player_60fps === false) )
if ( this.dataset.value && this.dataset.value === false)


..And just in case you like the insight: this was the only issue on Thursday. = `v4.raszpl` went through well. (While extra uninstalls will be from the "no 60fps", some excitement (reinstalltions) with no bugs also just happen at any update, both of which was very worth it.) chrome webstore uninstalls + Through our log for the uninstall.html, i might/we could react faster than bug reports, if it goes up.
Screenshot_788

( https://improvedtube.com/uninstalls-hours-days-usatime-updated-every-3-minutes )

@raszpl
Copy link
Contributor Author

raszpl commented May 21, 2023

Looks like it can be removed & isnt by the author of satus; (Seems @dodieboy first used it https://github.com/code-charity/youtube/pull/1020/files#diff-3df6958c77d615b266b54fcf12fadf313e7c1e4b168f0c61ea8c3424eb39eb8c )

good to know. I need to look how "active features works", how does it know when something is enabled/disabled. Extension should really use same method to know when some option is worth saving in the config. This way turning something ON and OFF again wont pollute config with useless "something:disabled", only truly enabled options will be stored.

exactly😊 ( #1685 ) if (ImprovedTube.storage.player_60fps === false) (or to avoid JS errors: (ImprovedTube.storage.player_60fps && ImprovedTube.storage.player_60fps === false) ) if ( this.dataset.value && this.dataset.value === false)

player_60fps is an unfortunately named option :(, we dont really enable 60fps, we disable blocking >30fps. Maybe it should be turned into Framerate 'select' with options: [All, limit to 50, limit to 30].
Same deal with all "autoplay" options, Imo more logical would be 'Disable Autoplay' disabled by default. After all no one needs external ways of enabling autoplay on YT :) Im of the position freshly installed extension should have no options showing being turned on by default to better reflect reality, its not the extension that turns them on, extension is only able to turn them off.

One thing I think is missing from this extension is a changelog, every update should come with one so users can quickly see if new problem they are experiencing might be caused by recent change, or discover new features.

@raszpl
Copy link
Contributor Author

raszpl commented May 21, 2023

exactly😊 ( #1685 ) if (ImprovedTube.storage.player_60fps === false) (or to avoid JS errors: (ImprovedTube.storage.player_60fps && ImprovedTube.storage.player_60fps === false) ) if ( this.dataset.value && this.dataset.value === false)

just for clarity neither this
!ImprovedTube.storage.player_60fps
nor this
ImprovedTube.storage.player_60fps === false
has any potential of generating js errors when player_60fps doesnt exist. ImprovedTube.storage.player_60fps turns from true/false into undefined and the only resulting problem is forcing 30fps on all videos, doubt anyone would immediately notice and uninstall over this without flipping "Allow 60fps" even once to test :)
How was this the cause for #1689 ? is YT serving some videos in 1080@60 only without 1080@30 version?

What I think might have been the problem were people who disabled all available video codecs while that option was pure placebo and didnt work, and when you pushed patch with my fix all of a sudden it started working and "Your browser can't play this video." #1695 . Unintended consequences :( with hindsight that update should have reset [block_vp9, block_h264, block_av1] options. Changelog announcing the change might have helped as well. Ill try to do better next time.

@ImprovedTube
Copy link
Member

ImprovedTube commented Jun 13, 2023

hi, so sad i missed the answers again! (BTW wondering to invite every user to discord (+github / or at least the right few % if possible) and sync discord with (web-)IRC allowing guests too)

need to look how "active features works", how does it know when something is enabled/disabled. Extension should really use same method to know when some option is worth saving in the config. This way turning something ON and OFF again wont pollute config with useless "something:disabled", only truly enabled options will be stored.

yes & as you said before, we aren't required to keep anything "on" by default, but could feature those that improve performance or avoid distractions differently in their own lists / search shortcuts.

  • And active features is currently not showing the extra buttons as activated 😲 (the only thing we activated by default,).

60 fps

( just noticing you said it here too👍😀)

changelog.

Yes, or again at least some % of users who will be interested, like those who installed the beta version. like https://github.com/code-charity/youtube/releases + there could be a major note, yearly or so, for every user (including a fundraiser for bug bounties, charity, ...)


(or to avoid js errors browser consoles logging undefined variables ..)

is YT serving some videos in 1080@60 only without 1080@30 version?

exactly. And (!ImprovedTube.storage.player_60fps) blocked 60 FPS for everybody,
that's why the moment seemed lucky to show you #1685

  • happens for many Pull Requests. Also for this pretty (+ambitious) feature Comments: Sidebar update #1697 @D-Rekk, we currently need to go either:
    html .. [it-header-position=normal], :not([it-header-position] { .... ( or
    html .. :not([it-header-position^=h] { .... - h for hover, hidden,...)

    or could have saved the author from the following:

[it-header-position=normal] #columns, /* BUG: if header has never been changed back and forth to normal, [it-header-position=normal] selector isn't applied */ "

ImprovedTube added a commit that referenced this pull request Jan 12, 2024
ImprovedTube pushed a commit that referenced this pull request Jan 12, 2024
fix for "After installation, h.264 needs to be clicked twice before it reacts and the modal appears"
#1681
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.

2 participants