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

Add option to hide YouTube Shorts from the feed #9049

Closed
wants to merge 5 commits into from

Conversation

chowder
Copy link

@chowder chowder commented Oct 1, 2022

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

Adds an options to hide YouTube Shorts from the feed.

Before/After Screenshots/Screen Record

  • Before:

  • After:

Fixes the following issue(s)

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

val streamItems = if (event is SuccessResultEvent || event is IdleEvent)
feedDatabaseManager
.getStreams(groupId, showPlayedItems, showFutureItems)
.blockingGet(arrayListOf())
.filter { s -> showShorts || s.stream.duration > 0 }
Copy link
Member

Choose a reason for hiding this comment

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

Please explain this filter. Do shorts have an unknown duration (i.e. -1))?

What about other services? When there is an unknown duration for e.g. a media.ccc.de or PeerTube stream, they are removed by your filter, too. I suggest to only remove streams with the YouTube service id.

@AudricV
Copy link
Member

AudricV commented Oct 9, 2022

This is a non-viable solution, as it is planned in the future to extract shorts duration from channels using the accessibility data. Also, this would hide any video if the duration extraction fails in the future.

Note: YouTube will roll out in the future dedicated shorts and live tabs, as you can see in this video.

@chowder
Copy link
Author

chowder commented Oct 17, 2022

Thanks for the comments, both.

I spent some more time on this, and struggled to find a way to elegantly implement this, so I raised this PR in NewPipeExtractor to be able to identify streams that are short-form content (i.e. content in the style of TikTok, YouTube Shorts, or Instagram Moment videos), that this PR is contingent on.

I'd like to keep this PR open in the meantime to provide some context for the NewPipeExtractor PR, and for any tangential discussions, e.g.

  • whether such setting should be applied globally (channel feeds, trending feeds), or only to subscription feeds
  • whether such toggle would be better placed in the settings view
  • a more appropriate icon to use for the toggle

@killerrook killerrook mentioned this pull request Oct 19, 2022
6 tasks
@Douile
Copy link
Contributor

Douile commented Nov 1, 2022

It would be nice if this was a unified option to filter types of content: a filter content menu where you can tick on/off

  • regular video
  • shorts
  • live streams
  • possibly future videos (premiers), although this would duplicate the show future content button in feed
  • other types of content from other services

@opusforlife2
Copy link
Collaborator

although this would duplicate the show future content button in feed

@Douile This is the exact button that would need to be changed into a content filter menu. We shouldn't have the same functionality in different buttons/menus.

other types of content from other services

This raises an important point: the filter should be service-specific. So when you change the service from the hamburger menu, the filter menu should change accordingly. Also, it should remember the last set of selections for that service.

whether such setting should be applied globally

@chowder How about only feeds for now?

whether such toggle would be better placed in the settings view

As mentioned above, the "hide premieres" button could be changed into a menu with check boxes. Probably, the last ticked item in the check box shouldn't be untick-able.

a more appropriate icon to use for the toggle

A filter icon? Is that available?

Honestly, I would prefer the eye icon, which is unfortunately taken up by the "hide watched videos" button. But I don't know if we could find another icon for that button, either. Something like a play icon with a tick at the bottom.

Or maybe the "watched videos" button could become just one more check box in the filter menu itself, though visually separated from the content-type check boxes. After all, the functionality is the same - hiding certain videos based on a condition. In that case, the eye icon could be used for this single unified drop-down.

@opusforlife2
Copy link
Collaborator

Question: should the filter be feed-specific as well? e.g. you might only want regular videos from a news channel, but livestreams from a gaming channel. And this preference is unlikely to change much. So someone who frequently switches between feeds would have to keep adjusting the filters, which seems inconvenient.

@TobiGr TobiGr added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface and removed GUI Issue is related to the graphical user interface labels Nov 9, 2022
@sonarcloud
Copy link

sonarcloud bot commented Nov 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Stypox
Copy link
Member

Stypox commented Dec 18, 2022

Thank you!
I think there are some database-related issues, since the 5.json schema shouldn't change. Could you revert changes in that file?
Also, at the moment there does not seem to be any code that actually sets the "is short content" value to stream entities, and so that field is always set to false by default. You will want to use TeamNewPipe/NewPipeExtractor#946 for this :-)

@chowder chowder marked this pull request as draft December 22, 2022 00:14
@chowder
Copy link
Author

chowder commented Dec 22, 2022

Thanks for reviewing, @Stypox!

Indeed NewPipeExtractor doesn't support the new channel tabs layout at the moment; as such, shorts are currently not being extracted, making this PR somewhat pointless (for now).

I'll mark this as a draft until TeamNewPipe/NewPipeExtractor#951 makes further progress.

@magicweirdoo

This comment was marked as duplicate.

@SameenAhnaf
Copy link
Collaborator

@chowder As channel tabs' PR is merged, you may resume or discontinue working on #8069.

@AudricV AudricV changed the title Add option to hide YouTube Shorts Add option to hide YouTube Shorts from the feed Sep 22, 2023
@SameenAhnaf
Copy link
Collaborator

Closing due to no response from author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants