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

Local API: Extract playlists on the auto-generated "Music" channel #5250

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

absidue
Copy link
Member

@absidue absidue commented Jun 10, 2024

Local API: Extract playlists on the auto-generated "Music" channel

Pull Request Type

  • Feature Implementation

Description

This pull request adds support for extracting and displaying playlists on the auto-generated system "Music" channel. Unlike #5241 which adds support for a whole category of channels, this pull request is only for one specific one.

I also noticed that some of the playlists listed there were showing NaN views on the playlists page, so I fixed that so it correctly shows 0 views when YouTube says No views.

Screenshots

music-channel-with-playlists

Testing

Open https://www.youtube.com/music in FreeTube and notice the playlists tab.

Desktop

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 0050909

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 10, 2024 21:54
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 10, 2024
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

@@ -340,7 +340,7 @@ export default defineComponent({
this.playlistDescription = result.info.description ?? ''
this.firstVideoId = result.items[0].id
this.playlistThumbnail = result.info.thumbnails[0].url
this.viewCount = extractNumberFromString(result.info.views)
this.viewCount = result.info.views.toLowerCase() === 'no views' ? 0 : extractNumberFromString(result.info.views)
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion (non-blocking): We have a few places in the code where we use extractNumberFromString and it ends up getting used as NaN. I get from an "API" level why NaN is an appropriate return value, but it is this behavior that ultimately results in us displaying NaN throughout the app. I'd recommend returning 0 instead of NaN from that function in both the not-string case and NaN parseInt case just to cover the swathe of edge cases more easily.

Copy link
Member Author

Choose a reason for hiding this comment

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

While you are correct that in many places we don't handle NaN and just display it to the user, in other places we actually rely on it, so we know when to hide UI elements for example.

So we either need to create a separate wrapper function the converts the NaN to zero in the places where that is desired or add an extra parameter to control when the fallback should be zero.

@FreeTubeBot FreeTubeBot merged commit eddee49 into FreeTubeApp:development Jun 11, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 11, 2024
@absidue absidue deleted the music-playlists branch June 11, 2024 12:20
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