-
Notifications
You must be signed in to change notification settings - Fork 842
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
Add Tabs to subscriptions page for live streams and shorts #3725
Conversation
closes #2852 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors i've found
IV-API.mp4
IV-API-RSS.mp4
IV-API-RSS-2.mp4
Local-API.mp4
Local-API-2.mp4
Local-API-RSS.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be RSS
case 1: | ||
if (process.env.IS_ELECTRON && this.backendFallback) { | ||
showToast(this.$t('Falling back to the local API')) | ||
return this.getChannelLiveLocal(channel, failedAttempts + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return this.getChannelLiveLocal(channel, failedAttempts + 1) | |
return this.getChannelLiveLocalRSS(channel, failedAttempts + 1) |
const channelInstance = await getLocalChannel(channel.id) | ||
|
||
if (channelInstance.has_live_streams) { | ||
const liveTab = await channelInstance.getLiveStreams() | ||
|
||
if (liveTab.videos === null) { | ||
this.errorChannels.push(channel) | ||
return [] | ||
} | ||
|
||
return parseLocalChannelVideos(liveTab.videos, channelInstance.header.author) | ||
} else { | ||
return [] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reduce no. of requests by half like video tab (2 => 1 request per channel)
const channelInstance = await getLocalChannel(channel.id) | |
if (channelInstance.has_live_streams) { | |
const liveTab = await channelInstance.getLiveStreams() | |
if (liveTab.videos === null) { | |
this.errorChannels.push(channel) | |
return [] | |
} | |
return parseLocalChannelVideos(liveTab.videos, channelInstance.header.author) | |
} else { | |
return [] | |
} | |
const entries = await getLocalChannelLiveStreams(channel.id) | |
if (entries === null) { | |
this.errorChannels.push(channel) | |
return [] | |
} | |
return entries |
Plus adding following code to src/renderer/helpers/api/local.js
export async function getLocalChannelLiveStreams(id) {
const innertube = await createInnertube()
try {
const response = await innertube.actions.execute(Endpoints.BrowseEndpoint.PATH, Endpoints.BrowseEndpoint.build({
browse_id: id,
params: 'EgdzdHJlYW1z8gYECgJ6AA%3D%3D'
// protobuf for the live tab (this is the one that YouTube uses,
// it has some empty fields in the protobuf but it doesn't work if you remove them)
}))
const liveStreamsTab = new YT.Channel(null, response)
// if the channel doesn't have a live tab, YouTube returns the home tab instead
// so we need to check that we got the right tab
if (liveStreamsTab.current_tab?.endpoint.metadata.url?.endsWith('/streams')) {
return parseLocalChannelVideos(liveStreamsTab.videos, liveStreamsTab.header.author)
} else {
return []
}
} catch (error) {
console.error(error)
if (error instanceof Utils.ChannelError) {
return null
} else {
throw error
}
}
}
}, | ||
|
||
getChannelVideosLocalRSS: async function (channel, failedAttempts = 0) { | ||
const feedUrl = `https://www.youtube.com/feeds/videos.xml?channel_id=${channel.id}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use the UULF playlist here instead of channel rss (it'll exclude live + shorts).
I'm also wondering if we should include an all tab?(This can be done in a different PR if needed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tab with refresh button = easier to get rate limited
No for me :S
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the all tab may only work if it uses rss in that case and the all tab should probably be added in a future PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It made sense when we didn't need a live tab but now that they're separate we may as well exclude them from the videos section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if RSS feed includes all types of videos (videos + live + short)
But I would consider RSS feed as a different type of "stream" and put into another tab / view
That's what I did in my branch of live implementation (but less complete than this PR)
} | ||
|
||
invidiousAPICall(subscriptionsPayload).then(async (result) => { | ||
resolve(await Promise.all(result.videos.map((video) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invidious endpoint for live streams returns non video entries too
Channel tested: https://www.youtube.com/channel/UCnKbs5xffaEzzdKdPZvfhdQ
resolve(await Promise.all(result.videos.map((video) => { | |
// Sometimes `channel`, 'category' entries are mixed in the result | |
resolve(await Promise.all(result.videos.filter(e => e.type === 'video').map((video) => { |
Hmmm what else could be broken :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better UX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're gonna hate me for this request but can u switch the placement of shorts and live?
So shorts in the middle and live to the right.
This way it is consistent with the order of the channel page tabs
Not sure if we want to address it here but i mention it anyway. For the Local API the Upcoming videos Live videos are shown first and then the Upcoming videos. IMO it would make more sense to show the Upcoming first and the the Live videos Local API + RSS strips the upcoming labels which is expected but the current live videos are not shown on top of the page anymore Local-API-RSS.mp4For the IV API the Live and Upcoming labels are stripped from the thumbnails IV API + RSS shows the same stuff as Local API + RSS IV-API-RSS.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot of code duplication but that could be fixed in a separate pull request later on, as that doesn't make any difference to the functionality provided by this pull request.
Including this in my own test build (with playlist too) |
…d instead of mounted
@efb4f5ff-1298-471a-8973-3d47447115dc Sorry pushed another commit 🙈. Now I'm actually done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, LGTM :)
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull requests will keep getting conflicts because people are translating the podcast and release tab strings. Either we speed run the approvals or wait a few weeks until everyone has translated stuff. |
Speed run lets goooooo |
Conflicts have been resolved. A maintainer will review the pull request shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested new stuff
@ChunkyProgrammer pls review ASAP as we dont want to get conflicts because of translating strings |
Hey guys, have you considered that some people would prefer to see all the video types from their subscriptions in one tab rather than to have to click every time to see if there are updates in the other tabs. Maybe the tabs can act as filters. And/or the behavior can be changed in the settings pane. The reason is some channels produce more normal videos, other focus on shorts and third mostly live videos. When I open my subscription page I want to see an overview of the updates. I don't want to have to click through every time I check for video updates. |
@alex-milanov See the discussion here as for why that is not possible without loosing functionality (the tabs don't act as filters, they are completely separate tabs): #4225 |
My comment was ignored in the other topic and it was closed and locked. I was also considering contributing but with this attitude my attention might be well spent elsewhere. Best of luck. |
Implementing support for every niche use case with a worse UX than the current implementation is not our goal no. Your cache idea wouldn't work either because there is no way to know if there is a new upload without firing requests off to YouTube, bypassing the cache. |
Worse UX is a very subjective statement. The default UX for the feed at least at 0.17.1 had all the video types combined in one feed. Which I guess some users got accustomed to. And I would argue that introducing tabs instead of "type filters" is actually a step in the wrong direction UX wise and this is actually part of what I am engaged in day to day basis. In any case the previous paragraph is an actual discourse with the goal of solving an issue or coming up with a more usable UI, subjective or not. My main issue was that the discussion was shut down in the other issue that you referred me to. That type of interaction is less that constructive. |
Tabs are a youtube change, not a freetube change. Youtube changed their channel page fundamentally, and so, comparing UI of previous versions of freetube is irrelevant to this problem. Previous versions of freetube expected youtube to work differently than it does now. The problem is that youtube no longer returns everything as one big feed, and it is important to remember that if you make too many requests, you will get rate-limited, so making extra requests isn't really a viable option. Even if you go back to 0.17.1 today, you will still only see the videos feed, no shorts, no live, and so, it is extremely unfair to compare the UX of 0.17.1 to 0.19.1 because the difference in UX you are describing is literally a change on YouTube's end, not FreeTube's. Freetube only conformed to the changes made by YouTube. |
Add Tabs to subscriptions page for live streams and shorts
Pull Request Type
Related issue
None IIRC
Description
Adds tabs to the subscriptions page to show live streams and YouTube Shorts along the usual videos. Shorts exclusively use RSS however live streams can be fetched through normal means and via RSS
Screenshots
Testing
Desktop