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

Edit: Defining forbidFocus() once in advance, instead of inside features #2761

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

4yman-0
Copy link

@4yman-0 4yman-0 commented Jan 1, 2025

Fix #2754

ImprovedTube.chapters tries to call ImprovedTube.forbidFocus which does not exists or is not a function. Tested with Chromium on Arch Linux and random youtube videos.

Fix code-charity#2754

`ImprovedTube.chapters` tries to call `ImprovedTube.forbidFocus` which does not exists or is not a function.
Tested with Chromium on Arch Linuxand random youtube videos.
@4yman-0 4yman-0 requested a review from ImprovedTube January 1, 2025 07:26
@4yman-0

This comment was marked as off-topic.

@ImprovedTube
Copy link
Member

thank you! @4yman-0 glad you are here! sorry this one seems to be a duplicate of #2750

@4yman-0

This comment was marked as resolved.

@ImprovedTube
Copy link
Member

ImprovedTube commented Jan 6, 2025

hi @4yman-0, i appreciated your thoughts in the code.
the bug #2754 was undone in #2750.

While defining a function for every user in every YouTube-tab is cheap,
i wanted to make it conditional (it just appears in 3 features yet.)
So i had defined it conditionally in functions.js:

if (document.documentElement.dataset.pageType === 'video'
	 && (ImprovedTube.storage.description === "expanded" || ImprovedTube.storage.transcript === true || ImprovedTube.storage.chapters === true )) { ImprovedTube.forbidFocus =  function (ms) }

however i undid this to undo the bug

if you like, we should check why, going back to Dec21, https://github.com/code-charity/youtube/tree/HEAD@%7B2024-12-21%7D, typos? timing?/#2415

@4yman-0
Copy link
Author

4yman-0 commented Jan 6, 2025

So i defined it conditionally in functions.js:

if (document.documentElement.dataset.pageType === 'video'
	 && (ImprovedTube.storage.description === "expanded" || ImprovedTube.storage.transcript === true || ImprovedTube.storage.chapters === true )) { ImprovedTube.forbidFocus =  function (ms) }

I'll close the pull request if that's the case :)

@4yman-0 4yman-0 removed the request for review from ImprovedTube January 6, 2025 12:15
@4yman-0 4yman-0 self-assigned this Jan 6, 2025
@ImprovedTube
Copy link
Member

ImprovedTube commented Jan 6, 2025

hi! :) if or when? you chose, i'm fine if this thread keeps evolving - or to hoard open PRs for decades

Or i can merge for our motivation (and remove the function from the other two features where it appears)

@ImprovedTube
Copy link
Member

Great team-work! 👍🎉

@ImprovedTube ImprovedTube self-assigned this Jan 6, 2025
@4yman-0
Copy link
Author

4yman-0 commented Jan 6, 2025

This PR sets forbidFocus initialy while your commits make improvedTube.chapters set forbidFocus every time it is called.

Can you review?

@ImprovedTube ImprovedTube changed the title Fix Chapters not showing after reload Edit: Defining forbidFocus() once in advance, instead of inside features Jan 7, 2025
@ImprovedTube
Copy link
Member

we should check why, going back to Dec21, https://github.com/code-charity/youtube/tree/HEAD@%7B2024-12-21%7D, typos? timing?/#2415

/why the now out-commented condition might not yet work at that point in the code

Copy link
Author

@4yman-0 4yman-0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good To Me!

@ImprovedTube ImprovedTube added the untested please test this label Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
untested please test this
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐞Chapters not show in sidebar
2 participants