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

Fix external player not opening on Subscriptions page #3172

Merged

Conversation

ArturWagnerBusiness
Copy link
Contributor

Fixing external player not opening on Subscriptions page

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

none

Description

There is no edge case for undefined in the function that opens external players in /src/renderer/store/modules/utils.js:openInExternalPlayer() so I found where the undefined came from and changed it into a null.
This is because if playlistId is set to undefined it will break as it expects null or string. Undefined makes it believe it is a playlist and ignores the videoId
This was not an issue in the recent version of FreeTube (0.18.0).

Screenshots

Screenshot_20230207_162049

Testing

After changes I tested:
Opening a video on subscriptions page. (worked)
Opening a video on watch page. (worked)
Openng a playlist on search page. (worked)

Desktop

  • OS: Linux
  • OS Version: 5.15.91-1-MANJARO (64-bit)
  • FreeTube version: c349d63 (Latest commit)

Additional context

none

If playlistId is set to undefined it will break /src/renderer/store/modules/utils.js:openInExternalPlayer() as it expects null or string. Undefined makes it believe it is a playlist and ignores the videoId
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) February 7, 2023 22:24
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Feb 7, 2023
@PikachuEXE PikachuEXE changed the title Changed undefined to null Fix external player not opening on Subscriptions page Feb 8, 2023
@PikachuEXE
Copy link
Collaborator

@ArturWagnerBusiness
The PR title look hard to understand so I use what you put inside the PR description

@PikachuEXE
Copy link
Collaborator

IMO I think instead of changing one value supplier (there might be more)
Just change the consumer to treat undefined same as null

I personally usually use variable == null (yes two equals) which is the same as variable === null || variable === undefined

@ArturWagnerBusiness
Copy link
Contributor Author

I found using grep -r "openInExternalPlayer" . (root directory just in case) and grepping "this.playlistIdFinal" that only ft-list-playlist.js and fl-list-video.vue uses the this.playlistIdFinal so others will not be effected.

Also usage of playlistIDFinal seems to be only passed to the externalplayer function OR put in a playlist string after being checked in a if (this.playlistIdFinal && ...) which will work whether undefined or null as both are falsy by javascript standard.
Screenshot_20230208_073735

I think this change should not cause any damage but if the "!==" were to be change to "!=" should other properties of the payload in openInExternalPlayer be changed too just in case?
image
There is few more item !== null above this code as well. Which is why I originally decided with the smaller alteration which seemed safer in my opinion.

Although the option of doing the bigger change in openInExternalPlayer could be good for future profing in case new code is added that will somehow pass an undefined.

P.S.: Only other components with openInExternalPlayer are fl-list-playlist and watch-video-info which both seem to only pass string or null.

@ArturWagnerBusiness
Copy link
Contributor Author

I can do the change in the openInExternalPlayer() and do some testing if that is something you would prefer. I don't know if I should create a different PR or just revert+extend this one.

@PikachuEXE
Copy link
Collaborator

I would say use variable == null for playlistId and other conditionals in /src/renderer/store/modules/utils.js:openInExternalPlayer()

Unless other devs have some use of null & undefined separately

auto-merge was automatically disabled February 9, 2023 01:36

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) February 9, 2023 01:36
@ArturWagnerBusiness
Copy link
Contributor Author

Reverted my first commit and changed the needed expressions in openInExternalPlayer()

Tested opening external player for single video:

  • Subscriber page
  • Watch page (main video and side videos)
  • Search page
  • Playlist page
  • History page

Tested opening external player for playlists

  • Search page

@FreeTubeBot FreeTubeBot merged commit 38fe74a into FreeTubeApp:development Feb 9, 2023
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Feb 9, 2023
@ArturWagnerBusiness ArturWagnerBusiness deleted the development branch February 9, 2023 18:58
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.

None yet

5 participants