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

Rework content and sort filter framework #904

Open
wants to merge 40 commits into
base: dev
Choose a base branch
from

Conversation

evermind-zz
Copy link

@evermind-zz evermind-zz commented Aug 19, 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.

Sort filters reworked

  • framework for creating filter items
  • implemented sort filters for each service
  • reworked content filters

Closes:

Superseeds

Questions

@opusforlife2
Copy link
Collaborator

Holy shit it's happening

@FireMasterK
Copy link
Member

I like the approach of using proto files with wire! Any reason why these classes are in git and not auto-generated, perhaps with a gradle plugin? See: https://github.com/square/wire/blob/master/wire-library/docs/wire_compiler.md

@evermind-zz
Copy link
Author

I like the approach of using proto files with wire! Any reason why these classes are in git and not auto-generated, perhaps with a gradle plugin? See: https://github.com/square/wire/blob/master/wire-library/docs/wire_compiler.md

They are not in git. They will be auto-generated by the wire gradle plugin. Why do you think they are?

@FireMasterK
Copy link
Member

I'm having a look at the Filter, FilterGroup, FilterItem, SearchFiltersBase, etc classes. They look auto-generated to me, are they not?

@evermind-zz
Copy link
Author

evermind-zz commented Aug 26, 2022 via email

@FireMasterK FireMasterK mentioned this pull request Aug 27, 2022
3 tasks
Copy link
Member

@FireMasterK FireMasterK left a comment

Choose a reason for hiding this comment

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

Looks good for the most part, I just had a look at the general changes and YouTube-specific changes.

The code currently fails checkstyle, which is why tests cannot currently be run. Two tests appear to be commented out, but that's a minor issue and can be fixed easily.

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Thank you for implementing this long awaited feature.
I briefly skimmed over the most important code changes.
I noticed that there are no tests for the newly added classes and methods yet.

I've added singletons for some *QueryHandlerFactory as we rely now on the same object sets during a search. Was there a good reason to always recreate those objects?

IIRC some of the link handlers were not stateless in the beginning. But I think they are stateless now. So converting those into singletons should be fine.

@evermind-zz evermind-zz marked this pull request as draft August 27, 2022 13:03
@evermind-zz evermind-zz force-pushed the content-sort-filter-framework branch 2 times, most recently from 4a7a7bb to 4c420fb Compare October 12, 2022 00:05
@evermind-zz evermind-zz marked this pull request as ready for review October 12, 2022 00:21
@evermind-zz evermind-zz requested review from TobiGr and FireMasterK and removed request for TobiGr and FireMasterK October 12, 2022 00:21
@evermind-zz evermind-zz force-pushed the content-sort-filter-framework branch from 4c420fb to b09eda4 Compare October 20, 2022 22:26
@evermind-zz evermind-zz force-pushed the content-sort-filter-framework branch from b09eda4 to bb97de7 Compare November 4, 2022 17:44
@evermind-zz evermind-zz requested review from FireMasterK and TobiGr and removed request for TobiGr and FireMasterK November 4, 2022 22:40
@Isira-Seneviratne
Copy link
Member

@evermind-zz I went through your changes a bit, and they look good to me overall. I've suggested a few changes; for some of these (the Utils encode and decode methods), a rebase will be needed.

evermind-zz and others added 12 commits December 30, 2023 10:31
… within the searchfilters

The class LibraryStringIds is a enum.
This enum's identify a message. The name of the enum constants
reflect the actual message. This message has to be transformed by the
library user into a meaningful string.
…nnull

Annotate methods in classes that got some signatures changed due to the
searchfilters integration
DividerItem was inserted in the content filter framework in the
NewPipeExtractor to have a section title for YoutubeMusic. But as
UI releated stuff seems a bit out of place in the Extractor I came
up with injecting the DividerItem aka section title in the frontend
without having to change too much in the frontend.
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Here you are. I finished rebasing, but left some fixup commits for clearness. Those will need to either be squashed or get a better commit description. I also kind of reviewed the code, but I will need to review it more.

Now there are only a couple of tests to fix. I pushed a commit to update mocks just so that tests don't fail for that reason. The first two failures in the image are caused by the emergency info filter not being used for YouTube search, while the last one is for PeerTube endpoints not being used anymore.
image

private static final String MUSIC_SEARCH_URL = "https://music.youtube.com/search?q=";
private YoutubeSearchQueryHandlerFactory() {
super(new YoutubeFilters());
}

@Nonnull
public static YoutubeSearchQueryHandlerFactory getInstance() {
Copy link
Member

Choose a reason for hiding this comment

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

I saw that the creation of link handler factory instances was being done with a lazy getInstance, before the rebase. Since in the meantime we also added getInstance in the current extractor, I kept the current version to reduce changes, and thus discarded the lazy initialization. However we might want to check if it's better to lazily initialize all link handler factories to reduce static initialization time (but this can be done in a separate PR).

Comment on lines +25 to +26
final List<FilterItem> contentFilter,
final List<FilterItem> sortFilter,
Copy link
Member

Choose a reason for hiding this comment

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

Some other @Nonnull and @Nullable annotations are missing in this file (and all derivate classes)

Comment on lines +20 to +21
@Nonnull List<FilterItem> contentFilter,
@Nullable List<FilterItem> sortFilter)
Copy link
Member

Choose a reason for hiding this comment

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

Why @Nonnull content filter but @Nullable sort filter?

Comment on lines +20 to +21
@Nonnull List<FilterItem> contentFilter,
@Nullable List<FilterItem> sortFilter)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we just use one array with FilterItem and stop making the distinction between content and sort filters? What is the advantage of having two separate arrays?

Copy link
Author

Choose a reason for hiding this comment

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

a content filter might have a different set of sort filters.

Copy link
Member

Choose a reason for hiding this comment

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

But there can already be content filters from various available filter groups, so why can't sort filters be just another filter group? Maybe I'm missing something

Copy link
Author

Choose a reason for hiding this comment

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

I'll have to review my code again. I think I thought about that.

Comment on lines 21 to 23
public static final String SEARCH_ENDPOINT_PLAYLISTS = "/api/v1/search/video-playlists";
public static final String SEARCH_ENDPOINT_VIDEOS = "/api/v1/search/videos";
public static final String SEARCH_ENDPOINT_CHANNELS = "/api/v1/search/video-channels";
Copy link
Member

Choose a reason for hiding this comment

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

I think this broke PeerTube filters. In theory using resultType=videos/playlists/channels should work, however it doesn't seem to, see query with resultType and query with different endpoints. PeerTube on the web uses the endpoints, too.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed with 70a2b63

case MUSIC_ARTISTS:
return "";
default:
return "8AEB";
Copy link
Member

Choose a reason for hiding this comment

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

We need to update the protobuf file in order to include the "allow emergency requests" field, and then always set that field to true. I didn't do this while rebasing because I have no idea how the protobuf file is obtained.

Copy link
Author

Choose a reason for hiding this comment

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

could you point me more information about the emergency requests? Where is it handled in eg. the browser youtube client.

@Stypox Stypox force-pushed the content-sort-filter-framework branch from b938a3c to e81796f Compare December 30, 2023 09:33
@evermind-zz
Copy link
Author

@AudricV

I will h

Here you are. I finished rebasing, but left some fixup commits for clearness. Those will need to either be squashed or get a better commit description. I also kind of reviewed the code, but I will need to review it more.

Now there are only a couple of tests to fix. I pushed a commit to update mocks just so that tests don't fail for that reason. The first two failures in the image are caused by the emergency info filter not being used for YouTube search, while the last one is for PeerTube endpoints not being used anymore. image

thx for rebasing. I might also have some fixes/changes in my 'BraveNewPipe extractor fork. I'll going to look into it.

'ALL' option not possible as Peertube uses different
endpoints for video/channels/playlist search
…able encountered IOException

The following Exception crashes NewPipe. Adding Serializable to FilterItem solves it:

java.lang.RuntimeException: Parcelable encountered IOException writing serializable object
(name = org.schabi.newpipe.extractor.linkhandler.ListLinkHandler)
...
Caused by: java.io.NotSerializableException: org.schabi.newpipe.extractor.search.filter.FilterItem
@Isira-Seneviratne
Copy link
Member

Isira-Seneviratne commented Jan 4, 2024

Base64 support has been added to the desugar library, so we'll be able to remove the uses of ByteString from this PR.


public FilterContainer(@Nonnull final FilterGroup[] filterGroups) {
this.filterGroups =
Arrays.stream(filterGroups).collect(Collectors.toCollection(ArrayList::new));
Copy link
Member

@Isira-Seneviratne Isira-Seneviratne Jan 4, 2024

Choose a reason for hiding this comment

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

Suggested change
Arrays.stream(filterGroups).collect(Collectors.toCollection(ArrayList::new));
Arrays.asList(filterGroups);

@evermind-zz
Copy link
Author

At the moment I have not enough time to look into it. But I will do it whenever I have some time for it.

@AudricV
Copy link
Member

AudricV commented Mar 14, 2024

could you point me more information about the emergency requests? Where is it handled in eg. the browser youtube client.

@evermind-zz As I can't anwser you as a comment as I have a pending review of your PR (in which I also explained you where to implement this property), see #1127.

I will try to finish the review soon, but I can't promise you anything.

@HJX-001

This comment was marked as resolved.

@HJX-001

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

8 participants