fix(Home): hide Trending category #7675
Conversation
FineFindus
force-pushed
the
chore/remove-trending-default
branch
from
f36b8d1 to
9f83aa9
Compare
Weird, it still works fine for me as of today 👀 |
| ) | ||
| override fun getTrendingCategories(): List<TrendingCategory> = trendingCategories.keys.toList() | ||
| override fun getTrendingCategories(): List<TrendingCategory> = | ||
| trendingCategories.keys.drop(1).toList() |
There was a problem hiding this comment.
Perhaps we should fully remove the TRENDING enum entry instead of dropping it manually everywhere? Likely, we should just disable trends for Piped (which is the only user of the TRENDING category after these changes if I see that correctly)
| }, | ||
| PreferenceMigration(1, 2) { | ||
| // select a random category as the new value | ||
| putString(PreferenceKeys.TRENDING_CATEGORY, TrendingCategory.entries.drop(1).random().name) |
There was a problem hiding this comment.
I'd default to LIVE, but that's just personal preference.
Anyways I'd like to avoid making it random, it should perhaps be consistent for all users.
| // cache the loaded trends in the [TrendsViewModel] so that the trends don't need to be | ||
| // reloaded there | ||
| trendsViewModel.setStreamsForCategory(TrendingCategory.TRENDING, streamItems) | ||
| trendsViewModel.setStreamsForCategory(category, streamItems) |
Knowing YouTube that's either because it's a gradual roll-out or they're A/B testing whether it can be rolled-out, but I guess in either case it doesn't hurt to have this ready :) |
|
It makes sense to merge the cache fix already before creating the release, I don't have a preference when we merge the other changes though (I'm fine with merging it now already too). |
|
Instead of deleting the category, can we just merge all category (gaming, music, live, etc) into this |
|
No, as that would require us to make |
| // cache the loaded trends in the [TrendsViewModel] so that the trends don't need to be | ||
| // reloaded there | ||
| trendsViewModel.setStreamsForCategory(TrendingCategory.TRENDING, streamItems) | ||
| trendsViewModel.setStreamsForCategory(category, streamItems) |
There was a problem hiding this comment.
Related: I noticed we currently only set the first 10 trending streams to the view model (because showTrending only gets passed the 10 first items).
Ref: #7695
There was a problem hiding this comment.
Sorry for chiming in, I accidentally set the category to podcast, and my podcast is not showing (related: #7602). Since the returned StreamItems is null, this section is kept hidden. And the only way to change the category back is via trendingTV, and since it's hidden, I cant change it back.
Maybe kept trendingTV shown and put a placeholder in trendingRV when the returned list is empty?
There was a problem hiding this comment.
I agree, sounds like a good idea.
|
Trends are gone for me too now since yesterday, so I agree with merging this now. |
|
Oddly enough they have re-appeared for me, so I guess it's A/B-Test/gradual rollout. Though I don't oppose merging, since it's still something that will be removed in the long-run. |
Bnyro
force-pushed
the
chore/remove-trending-default
branch
from
9f83aa9 to
c4b47df
Compare
As the `Trending` trending category is finally fully shutdown, hide the category from users and migrate any existing users to another category. Also fixes an issue, where the wrong category items could be cached from trending.
Bnyro
force-pushed
the
chore/remove-trending-default
branch
from
c4b47df to
3c08586
Compare
Please open a new issue about that. |
3 participants
As the
Trendingtrending category is finally fully shutdown (i.e. the API doesn't work for me for the last few days anymore), hide the category from users and migrate any existing users to another category.Also fixes an issue, where the wrong category items could be cached from trending.
Happy to also use another choice for the default category value.