-
Notifications
You must be signed in to change notification settings - Fork 889
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 Distraction Free Settings -> Hide Upcoming Premieres #2853
Conversation
#2849 might require some time to be merged |
Yes, if #2849 looks like it should come after I can definitely clean this one up with the aim of landing first and having no stubs in the meantime. If that looks likely then I'm also considering moving the new option (and related data/getter additions) higher in the list to sit alongside Hide Live Streams, both because it makes sense for them to have adjacent positions and because it should help avoid many-but-not-all merge conflicts when #2849 needs to rebase on top. |
I've done some more testing and I think this is ready for a look. First time contributor playing it by ear on style since it looks like prettier hasn't been used in awhile. Glad to make tweaks as needed--let me know! |
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 locally
Thanks for your work!
} | ||
if (this.hideUpcomingPremieres && | ||
(data.isUpcoming || data.premiere || data.durationText === 'PREMIERE' || | ||
(data.isRSS && data.viewCount === '0'))) { |
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.
Not sure if we should include the rss check as there could be videos that aren't premiers that will be hidden
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 this is (data.isRSS && data.viewCount === '0')
Meaning if using RSS feed and view count is zero, assume it's premiere
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.
Pika is correct. This is the only feasible way to remove them in the case of RSS data. There are currently no other flags to pivot on without making an additional request per video.
With RSS data as my only working option at the moment, that line (and the equivalent in Subscriptions.js) is the only reason this feature does anything for me.
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've added some additional comments alongside this to clarify.
src/renderer/components/ft-list-lazy-wrapper/ft-list-lazy-wrapper.js
Outdated
Show resolved
Hide resolved
Head branch was pushed to by a user without write access
Add Distraction Free Settings → Hide Upcoming Premieres
Pull Request Type
Related issue
closes #507
see also #1070, #2755
Description
Add a new boolean setting to Distraction Free: "Hide Upcoming Premieres."
When enabled, removes non-live future premieres from subscription and channel pages.
YT-side data unpredictably flips some streams that get "waiting" viewers from premieres into live streams and back. The live streams introduced can be removed by also enabling the "Hide Live Streams" setting.
Seeking feedback: Due to the weird shapeshifting streams issue above, and in order to avoid adding yet another setting, IMO there's a good argument for combining this new option with "Hide Live Streams" and having one switch do both. If you want one on, you probably want the other. I definitely need both myself. Made it a separate switch for now but happy to make the change if maintainers think combining makes sense.
Screenshots
New setting
An affected video
Setting OFF
The first item is our target.
Setting ON
And it's gone.
Testing
I've tested the new setting using
npm run dev
against Local API (no fallback) due to Invidious API not cooperating at present, with Fetch Feeds from RSS both OFF and ON.For a quick test:
I've tested Subscriptions, Channel and Search pages. If you see areas where I'm not removing videos as appropriate please let me know.
Desktop
Additional context
This PR conflicts with #2849 and the later merge will need rebasing/fixing. I'm borrowing their excellent showResult() addition to ft-list-lazy-wrapper.js since it's easier to work with than the previous v-if one-liner.