Skip to content
This repository has been archived by the owner on Jun 5, 2023. It is now read-only.

[niconico] Add content filter and sort filter #18

Closed
wants to merge 4 commits into from
Closed

[niconico] Add content filter and sort filter #18

wants to merge 4 commits into from

Conversation

34j
Copy link
Contributor

@34j 34j commented Sep 13, 2022

  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

This change adds content filters (which allows full-text search for tags) and sorting filters in niconico.
As of now sorting filters cannot be manipulated from the UI and waiting for TeamNewPipe/NewPipe#8837 to be merged.

@InfinityLoop1308
Copy link
Owner

Thanks for your contribution! This is a fantastic feature. I will check it once the upstream PR is merged.

@34j
Copy link
Contributor Author

34j commented Sep 13, 2022

Just in case my terrible English is misinterpreted, even if the PR above are not merged, (and even if it were merged, some changes would be needed.) the change for content filters are still valid and a full-text search capability for tags will be added, and do not cause any errors regarding to sorting filters. I included the code for sorting filters because it was after implementing it that I realize that it does not make sense (although there is such a function to override) and it took some effort, but if that is the problem, I will address it immediately (and I think I should do that).

@InfinityLoop1308
Copy link
Owner

Sorry for late response. I am really busy these days.

That seems good, but there is one thing. "Tags" is a little bit confusing and I think it is better to use "TagsOnly".
Also please ignore my fault of lowercase. It should be "All" and "TagsOnly" instead of all lowercase.

I noticed you have cloned the client. It would be nice if you can merge the upstream PR so that we can have a new feature and make use of (and test) the sorting filters. That PR is stuck now and I don't think it would be finished in ~2 months.

@34j
Copy link
Contributor Author

34j commented Sep 15, 2022

Thank you for taking the time to reply.

First, the reason why I set "tags" to snake_case is in reference to YouTubeSearchQueryHandlerFactory. They will be translated in NewPipeEnhanced-client's strings.xml and shown capitalized if the locale is English. However, if it is too much trouble to add them, I think there is a lazy implementation that capitalizes "tags". (And that is why I went uppercase in the first place.) If you have already thought this far, sorry.

The reason I forked the client was to implement another feature (I will open a issue or a PR later.), and I am not too willing to rush into merging unfinished code right now.

@InfinityLoop1308
Copy link
Owner

That's OK. Working on it.

InfinityLoop1308 added a commit that referenced this pull request Sep 16, 2022
…am PR #904(evermind-zz: content and sort filter framework) and refactor BiliBili and NicoNico service to fit in it. Also partially merge extractor PR #18 (34j: Add content filters).

Co-authored-by: 34j <55338215+34j@users.noreply.github.com>
InfinityLoop1308 added a commit that referenced this pull request Sep 16, 2022
Co-authored-by: 34j <55338215+34j@users.noreply.github.com>
@InfinityLoop1308
Copy link
Owner

Migrate to #20

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants