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

Implement Channel-Specific Category Selection #1261

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zedseven
Copy link
Contributor

@zedseven zedseven commented Apr 3, 2022

Adds channel-specific category selection, and makes the improvements necessary under the hood to make it possible.
This PR was created with a lot of @mchangrh's help. Thank you.

This adds ctrl-click functionality to the whitelist button in the popup. This opens channel-specific settings and creates a new channel entry if it's not present.

In order for this to work properly, it requires the Force Channel Check Before Skipping option to be enabled.

This was my first time working with TypeScript and React, so please let me know if I've done something incorrectly.

This PR is the reason for #1249 and #1250.

Notes

This PR makes the following changes to support channel-specific categories:

  1. Removes the whitelistedChannels config property, integrates channel whitelisting with the new channelSpecificSettings property, and migrates old configs to the new format.
  2. Adds a new Disabled enum option for CategorySkipOption, which is set to -1. (so it doesn't mess up existing option values stored in configs)
    This was required so that categories could be disabled for specific channels, since previously the extension disabled categories by removing them from the list. (which doesn't work for inherited values)
  3. Fetches the channel name at the same time as when it fetches the channel ID. This is used for display in the channel dropdown.
  4. The Behavior table header now has a defined height, so it doesn't change height when the two-line colour headings disappear.
  5. The hover text for the whitelist button in the popup has been changed, since there's already a notice when a channel is first whitelisted.
  6. Fixes a bug I found where shouldSkip would return true for full-video segments on music videos if autoSkipOnMusicVideos was enabled.

New Localisation Values

The following values are new and at the moment only have en localisation keys:

  • removeChannelSettingsButton: The label of the button for removing channel-specific settings.
  • inherit: The Inherit skip option for channel-specific category selections.
  • channelSettingsPopup: The hover text for the whitelist button that mentions to Ctrl-click the button for channel-specific settings.
  • forceChannelCheckRequired: The warning that channel-specific settings don't work properly if Force Channel Check Before Skipping is not enabled.

Known Jank

  1. More could probably be done to tell the user about the ctrl-click functionality, but I felt this was a good start.
  2. In the Behavior tab, scroll up the page a bit. Select a channel in the dropdown - the page will not scroll or move. If you now select the Global settings again, the page does scroll.
    It's the inconsistency that bothers me, not the scrolling.
    I have a feeling it's something to do with React DOM updates related to the re-appearing colour selection elements, but I'm not sure how or why.

  • I agree to license my contribution under LGPL-3.0 or my contribution is from another project with a license compatible with LGPL-3.0

To test this pull request, follow the instructions in the wiki.

@Oliver-Saer
Copy link

Awesome job here, thank you so much for implementing this! Hope it gets merged soon! ❤️

@ajayyy
Copy link
Owner

ajayyy commented Apr 19, 2022

In order for this to work properly, it requires the Force Channel Check Before Skipping option to be enabled.

Solution: Whenever a setting is changed, update a new list that saves the list of the union of all enabled categories. So, if filler is only enabled for one channel, still include it in that list. Use that list for all fetches

@ajayyy
Copy link
Owner

ajayyy commented Apr 22, 2022

Very nice implementation!

  • Whitelisting probably shouldn't auto-create a new channel config, only ctrl-click. Right now, whitelisting and then unwhitelisting leaves a zombie config in the options.

  • Maybe whitelisting should be removed entirely and replaced with this. Old whitelists still should be converted, but we can rename it to "Channel Overrides", and a single click opens the options page.

  • Either the options below the category selection need to also be synced with channels, or we should hide them when switching off global config to prevent confusion

@zedseven
Copy link
Contributor Author

zedseven commented May 6, 2022

Thank you!

I just wanted to mention that I haven't forgotten about this and I'm hoping to look at it next week. Just been very busy with work.

@wolph
Copy link

wolph commented May 6, 2022

I've just tested it and it's working great for me. Thank you!

@@ -38,6 +38,7 @@ export interface VideoDurationResponse {
}

export enum CategorySkipOption {
Disabled = -1,

Choose a reason for hiding this comment

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

❤️

@ajayyy ajayyy marked this pull request as draft July 22, 2022 17:57
@Splarkszter
Copy link

To test this pull request, follow the instructions in the wiki.

Artifacts expired i cannot download :(

@wolph
Copy link

wolph commented Apr 9, 2023

@zedseven

Thank you!

I just wanted to mention that I haven't forgotten about this and I'm hoping to look at it next week. Just been very busy with work.

Little bump. This feature would be very useful :)

@Splarkszter
Copy link

Little bump. This feature would be very useful :)

+1

@PasttaTypo
Copy link

To test this pull request, follow the instructions in the wiki.

Artifacts expired i cannot download :(

Yeah

@eirnym
Copy link

eirnym commented Jan 25, 2024

@zedseven any news?

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.

8 participants