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

Small fix & updated video and playlist URL-less popups #1844

Merged
merged 5 commits into from
Nov 20, 2023

Conversation

MAZ01001
Copy link
Contributor

@MAZ01001 MAZ01001 commented Nov 19, 2023

[1] A quick fix for a recent commit (e6a15c5) for #1842

document.querySelector('.custom-hamburger-menu')?.remove();
// Restoring the original padding:
document.querySelector('.html5-video-player')?.querySelector('.ytp-right-controls')?.style.paddingRight = '0'; }

When the first query selector for .custom-hamburger-menu fails, there is an optional chaining (?.) with remove, but the () comes after that, so it could be that it's trying to call null() with yields an error and the rest of the extension does not load...

With the second selector, everything's fine until it could be null.paddingRight, which is a problem, but even with an ?. there, it can be null = '0', which is, of course, also an error, so you need a temporary variable to check for null before setting it to something:

// [...]
document.querySelector('.custom-hamburger-menu')?.remove?.(); 
// Restoring the original padding:
const rightControls = document.querySelector('.html5-video-player')?.querySelector?.('.ytp-right-controls');
if (rightControls != null) rightControls.style.paddingRight = '0';      }
// [...]

[2] Updated URL-less window popup from 7fbb43f

Removed the title from the event calls since the window title automatically gets overwritten when the page loads.

Fixed: the variable named player in player.js to its correct name: ytPlayer (noticed this because the opened window had 0 width & height xD).

[3] Added URL-less popup window to playlist popup button

[4] Fixed video start-time of playlist popup

When the video opened is > 200th in the playlist, it uses the first video in the playlist (as expected), but it still has the same start-time as the initial video, so it could potentially skip to the end and immediately play the next, video for no apparent reason.

(now it removes the start time and video whenever that is detected).

fixed error when querySelecetors don't find their target (fixed chaining of `?.`)
formatting/refactoring and removed title change (gets changed on its own)
formatting and removed title
fixed variable name and removed title (also added a comment)
- fixed when opening video and playlist when video is >200th of playlist it starts at first entry (←normal), but at the start-time where the initial video was at, which might result in the video to skip to the end and immediately play the next (←problem)
- added the URL-less popup call to playlist button (to update popup window to a URL-less popup)
@ImprovedTube
Copy link
Member

ImprovedTube commented Nov 20, 2023

😮 😮 🥰 thanks!
1.) "If the object accessed or function called using this operator is undefined or null, the expression short circuits and evaluates to undefined instead of throwing an error."
(and existence of remove() does not need to be checked as it is a JS function.)

yes ("Error: Invalid left-hand side in assignment") - can be: document.querySelector('.html5-video-player')?.querySelector('.ytp-right-controls')?.style.setProperty('padding-right', '0', 'important')


2.) The title starts as the extension id which doesnt happen with the extensions for frameless windows. So that can still be improved.

  • adding something to it at last was unrelated to that and might just help some people when sorting & recalling browsing history. Can Append not to feel intrusive) ...
    //title
    chrome.tabs.onUpdated.addListener(function listener(tabId, changeInfo) {
    if (tabId === t.id && changeInfo.status === 'complete') {
    chrome.tabs.executeScript(t.id, {
    code: `document.title = "${message.title} - ImprovedTube Popup Player";`
    });
    chrome.tabs.onUpdated.removeListener(listener);
    ...

@ImprovedTube ImprovedTube merged commit f3d2959 into code-charity:master Nov 20, 2023
ImprovedTube added a commit that referenced this pull request Nov 20, 2023
@MAZ01001
Copy link
Contributor Author

MAZ01001 commented Nov 20, 2023

Interesting...I thought ?. also needed to be at remove?.() and style?.setProperty?.('...','...'), but apparently it's not necessary....good to know.

@MAZ01001
Copy link
Contributor Author

Ah I see

potentiallyNullObj?.a.b
This does not throw because evaluation has already stopped at the first optional chain

This is equivalent to
potentiallyNullObj === null || potentiallyNullObj === undefined ? undefined : potentiallyNullObj.a.b

And here I thought after resulting in null or undefined, it keeps going further in the chain.
Like how (potentiallyNullObj?.a).b will throw because the brackets get resolved first, then it continues with .b.

Should've RTFM ¯\_(ツ)_/¯

@MAZ01001 MAZ01001 deleted the small-fix-update-popups branch November 20, 2023 07:19
@ImprovedTube
Copy link
Member

ImprovedTube commented Nov 20, 2023

your work is superb! (while my commit i showed you had a bug & looked messy. )

ImprovedTube added a commit that referenced this pull request Dec 13, 2023
ImprovedTube added a commit that referenced this pull request Jan 12, 2024
ImprovedTube added a commit that referenced this pull request Jan 12, 2024
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