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 support for advanced content and sort filters in searches #8837

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

Conversation

evermind-zz
Copy link
Contributor

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

  • provide sort filters (and content filters) that are available by the supported platforms itself

There are 4 different UI to test

  • (1) dialog like (default)
  • (2) action menu look alike dialog (my favourite)
  • (3) action menu (legacy)
  • (4) chip dialog

To choose go to settings -> appearance -> 'Select Search Filter UI'

Notes:

  • UI (3) is slow but working (slow maybe because of misusing the action menu
Feedback needed
  • which UIs do you want (to merge)? I would like to keep 1 and 2 and 4. 3 seems to be also liked. So keep them all!. I will then rework the code as there is some duplication right now that should be eliminated first Any duplication has been removed.
Todo
  • fix sonar findings
  • fix your findings
  • string L10n

Before/After Screenshots/Screen Record

Select the UI (Ui 1) YouTube (UI 2) YouTube (UI 2) Peertube (UI 3) Live in action (UI 4) YouTube

Fixes the following issue(s)

Relies on the following changes

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

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug B 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 12 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@opusforlife2
Copy link
Collaborator

Maybe the UI could be changed from this giant scroll. Ideas?

@triallax triallax added the feature request Issue is related to a feature in the app label Aug 19, 2022
@triallax
Copy link
Contributor

triallax commented Aug 19, 2022

@opusforlife2 something like #2251 (comment) would be great in my opinion.

@opusforlife2
Copy link
Collaborator

@evermind-zz Is that UI something you would be interested in implementing? If not, we could always go with the current one as a minimum viable product and improve it later.

@evermind-zz
Copy link
Contributor Author

@evermind-zz Is that UI something you would be interested in implementing? If not, we could always go with the current one as a minimum viable product and improve it later.

For me the interaction with the current UI is quick and I would like to keep it. But I am willing to give implementing the other UI a shot. Maybe I will like the new UI too or I could make it configurable in settings if someone like me prefer the 'old way'

@MD77MD
Copy link

MD77MD commented Aug 23, 2022

first thank you for this... is it possible to have all the options to show up in one box without scrolling... same like the playback speed mene... with a proper layout for both portrait and landscape mode

also i think it is important to have [ok] [reset] buttons... so we can choose the filter /sort options in one go... not in multiple times and reloading results in each time

@TacoTheDank
Copy link
Member

For me the interaction with the current UI is quick and I would like to keep it. But I am willing to give implementing the other UI a shot. Maybe I will like the new UI too or I could make it configurable in settings if someone like me prefer the 'old way'

If we go with the current design for now, I would at least suggest using a dedicated filter icon (via Google's material icons). Having it be stuffed into the hamburger menu isn't very intuitive, IMO.

@evermind-zz
Copy link
Contributor Author

@MD77MD I do not really understand what you mean by you have to reload the gui. This version modifies the current menu filter behaviour. Here you first select everything in the menu and than you have to scroll to the top and push search inside the menu. Than the menu will disappear. Have you tested this PR here?

But nevertheless I will come up with a dialog style UI

@evermind-zz
Copy link
Contributor Author

For me the interaction with the current UI is quick and I would like to keep it. But I am willing to give implementing the other UI a shot. Maybe I will like the new UI too or I could make it configurable in settings if someone like me prefer the 'old way'

If we go with the current design for now, I would at least suggest using a dedicated filter icon (via Google's material icons). Having it be stuffed into the hamburger menu isn't very intuitive, IMO.

no matter which UI but a filter icon would be nice

@evermind-zz
Copy link
Contributor Author

@evermind-zz : Are there any news?

still working on it! I've rewrote the logic part and have a new UI! But my time is very limited so it will still take some time

@MD77MD
Copy link

MD77MD commented Sep 13, 2022

first thank you for this... is it possible to have all the options to show up in one box without scrolling... same like the playback speed mene... with a proper layout for both portrait and landscape mode

This is the closest thing to what i was trying to convey

Portrait mode:
108641489-b6429f00-74c9-11eb-8008-f086ab9a1574
from #5662 by @SameenAhnaf

landscape mode:
the same as the idea above but stacked like pic below
94719607-ae843180-0353-11eb-8646-a957e1d6f41e
from #110 (comment)

@MD77MD
Copy link

MD77MD commented Sep 13, 2022

With Ok, Reset buttons
20220914_013146

****** OR *****

190019317-4e61378c-ebb3-4e30-8509-1c42daffc22a

Personally, I prefer first one

@MD77MD
Copy link

MD77MD commented Sep 13, 2022

Also do you think adding #3824 , whether now or later, is possible?

@Angelk90
Copy link
Contributor

@evermind-zz, @MD77MD : Are there any news?

@opusforlife2
Copy link
Collaborator

@Angelk90 13 days since the last author reply is not nearly enough time to be asking for updates. This is closer to spam territory.

@evermind-zz
Copy link
Contributor Author

I'm still working on it. I'll post an update ASAP

@evermind-zz
Copy link
Contributor Author

evermind-zz commented Sep 29, 2022

Edit: comment obsolete. See first post
There are 3 different UI

  • (1) dialog like (default)
  • (2) action menu look alike dialog (my favourite)
  • (3) action menu (legacy)

To choose go to settings -> appearance -> 'Select Search Filter UI'

Notes:
All UI should work:

  • UI (1) is working. But I need some help. Sometimes in the spinners you can manage that the selected item is not visible
  • UI (2) is working! Also need some help with layouting.
  • UI (3) is slow but working (slow maybe because of misusing the action menu

The attached apk newpipe-with-search-filters.zip is just a snapshot of my working copy that is bit older than 0.24 release. But I've no time for a rebase atm.

In the next days/weeks I will update the code for this PR. Than you can drop me your insights of layouting it better programatically.

@nicoursi
Copy link

I would love to test it but the apk does not install on Android 11. Can you provide a suitable apk? Normally I can install the artifacts from the workflows, so there must be something wrong with this specific one.

@evermind-zz
Copy link
Contributor Author

evermind-zz commented Sep 29, 2022

I would love to test it but the apk does not install on Android 11. Can you provide a suitable apk? Normally I can install the artifacts from the workflows, so there must be something wrong with this specific one.

the problem is that this one has no signature: but with
adb install -t app-debug.apk
it should be installable. If not possible I will create a new one with a signature
Edit: @nicoursi above is now a hopefully working apk

@opusforlife2
Copy link
Collaborator

It became obvious in just a few tries that the dialog menu is far better to use compared to the action menu. Not only is it comfortably info dense without the need for lots of scrolling, it lets you change multiple settings before applying the filter/sort.

@Angelk90
Copy link
Contributor

Angelk90 commented Oct 1, 2022

@evermind-zz : It does not allow to be installed.

@evermind-zz evermind-zz force-pushed the content-sort-filter-framework branch 2 times, most recently from 047709d to 29f0182 Compare October 12, 2022 00:32
@evermind-zz
Copy link
Contributor Author

@evermind-zz : It does not allow to be installed.

The new 'artifact' one should work. See first post where to find it.

@Angelk90
Copy link
Contributor

Angelk90 commented Oct 14, 2022

@evermind-zz :
Tested on tablet, here are some screens:

Simple Dialog (default):
Screenshot_20221014-134856_NewPipe content-sort-filter-framework.jpg

Screenshot_20221014-134011_NewPipe content-sort-filter-framework.jpg

Screenshot_20221014-134033_NewPipe content-sort-filter-framework.jpg

Action Menu styled Dialog:
Screenshot_20221014-134133_NewPipe content-sort-filter-framework.jpg

Action Menu (legacy):
Screenshot_20221014-134200_NewPipe content-sort-filter-framework.jpg

@evermind-zz
Copy link
Contributor Author

@Angelk90 thx! Are screens in portrait mode?

@Angelk90
Copy link
Contributor

@evermind-zz : Yes, in portrait mode.

@evermind-zz
Copy link
Contributor Author

@evermind-zz : could you update the version to the recent one some times the videos freeze.
Use the latest BraveNewPipe version it is in sync with NewPipe 0.25.2

@AudricV
Copy link
Member

AudricV commented Dec 29, 2023

@evermind-zz

After updating your extractor changes, could you please rebase this PR on the dev branch (or merge it into yours if that's too difficult to rebase for you) and include your updated extractor in it? Thank you in advance.

which UIs do you want (to merge)? I would like to keep 1 and 2 and 4. 3 seems to be also liked. So keep them all!

I don't think this is a good idea to maintain 4 UIs of the search filters, especially with the current app codebase.

I personally prefer UIs 1 and 4, with minor changes (like converting them to an alert dialog instead of a dialog similar to the download one if possible and not done in a hacky way).

@AudricV AudricV changed the title Content sort filter framework Add support for advanced content and sort filters in searches Dec 29, 2023
@AudricV AudricV added multiservice Issues related to multiple services search Anything related to the search function labels Dec 29, 2023
@Stypox
Copy link
Member

Stypox commented Dec 30, 2023

I already started the extractor rebase (and almost finished, I just need to integrate 2 things), sorry for not writing that anywhere

…d content filters

SearchFilterLogic.java:
=======================
This class handles all the user interaction with the content and sort filters
of NewPipeExtractor.

The class works standalone to just get the default selected filters eg.
during init phase. See in SearchFragment#initializeFilterData()

BaseSearchFilterUiGenerator.java:
=================================
It extends SearchFilterLogic and is used as a base class to implement the UI interface
for content and sort filter dialogs eg. SearchFilterDialogGenerator or
SearchFilterOptionMenuAlikeDialogGenerator.
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.
Dialog looks similar to a action menu based approach but is faster.
This approach is more or less a hack but if all else fails. Could later
be dropped or right away.
…ragment

The ViewModel that hosts the search filters logic. It facilitates
the communication with the SearchFragment* and the
SearchFilter*DialogFragment based search filter UI's
…g into SearchFragment

There is also a configuration option to choose between different search
UI's
@evermind-zz
Copy link
Contributor Author

@evermind-zz

After updating your extractor changes, could you please rebase this PR on the dev branch (or merge it into yours if that's too difficult to rebase for you) and include your updated extractor in it? Thank you in advance.

which UIs do you want (to merge)? I would like to keep 1 and 2 and 4. 3 seems to be also liked. So keep them all!

I don't think this is a good idea to maintain 4 UIs of the search filters, especially with the current app codebase.

I personally prefer UIs 1 and 4, with minor changes (like converting them to an alert dialog instead of a dialog similar to the download one if possible and not done in a hacky way).

we can go with UI 1 and 4, but maybe we should decide after rebasing so everyone can have a hands on test.
I've started rebasing, I am nearly finished only have to test and will push tomorrow - also some fixes for the extractor will be pushed.

Happy New Year!!

@evermind-zz evermind-zz force-pushed the content-sort-filter-framework branch from da7b747 to 4eb71a8 Compare January 1, 2024 22:38
Copy link

sonarqubecloud bot commented Jan 1, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

10 New issues
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@evermind-zz
Copy link
Contributor Author

rebased finished and all seems to work properly.

@MD77MD
Copy link

MD77MD commented Jan 3, 2024

I vote for UI 4... but i wish selected option could be more highlighted... something like this but in red instead of gold color

190013629-40df50a5-3f42-4fe1-908d-3845ae177da7

@opusforlife2
Copy link
Collaborator

Instead of just red, it could be the service, colour, so red for YT, blue for Bandcamp, etc.

@TobiGr TobiGr added the size/giant PRs with more than 750 changed lines label Mar 29, 2024
@Angelk90
Copy link
Contributor

@evermind-zz : You can update, it seems the viewing of videos and channels no longer works.

@throwaway242685
Copy link

I also vote for UI 4.

it's the most intuitive one.

@Profpatsch
Copy link
Contributor

Hi all,

I wonder how we should proceed with this PR, since a lot of work has already been put into it, but it seems to have reached a stalemate.

Right now the project is refactoring everything to the new Jetbrains Compose UI, and in the process also from Java to Kotlin, and also to a new video player, so I’m afraid none of the code in here is going to apply in a short while.

We are also trying to get a designer on board, who could be more authoritative on UX decisions (based on objective research of course).

Would it be okay to convert this PR into an issue for the time being, and tackle it again after the rewrite?

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 GUI Issue is related to the graphical user interface multiservice Issues related to multiple services search Anything related to the search function size/giant PRs with more than 750 changed lines
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

Search filter UI