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.

Sign up for GitHub

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

Jump to bottom

Allow choosing which types of search suggestions to show #3546

Merged
merged 3 commits into from
Aug 24, 2021
Merged

Allow choosing which types of search suggestions to show #3546

merged 3 commits into from
Aug 24, 2021

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented May 6, 2020 •
edited
Loading

What is it?

  • Bug fix (user facing)
  • Feature (user facing)
  • Code base improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

This PR replaces the switch preference to enable/disable all search suggestions with a multiselect one, so that the user can choose which types of search suggestions he likes: local, remote, both, none. Since the previous preference stored a boolean while the new one stores a Set<String>, I had to create a migration function and call it in NewPipeSettings.init(). If the previous value was true, both local and remote suggestions are activated, otherwise they are both deactivated. I testes this and it works both when updating the app and when importing a database with the old setting.

Fixes the following issue(s)

Testing apk

@anpic could you test if this option suits you?
app-debug_1.zip

Agreement

@Stypox Stypox added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels May 6, 2020
@Stypox Stypox mentioned this pull request May 6, 2020
Closed
@Stypox Stypox added this to the 0.19.4 milestone May 7, 2020
@Stypox Stypox modified the milestones: 0.19.4, 0.19.5 May 28, 2020
@TobiGr
Copy link
Member

TobiGr commented Jun 23, 2020

I think we should also allow the user to stop showing local search suggestions. E.g. when showing something to another persion and using the search, one might not want to expose the search history.

@Stypox
Copy link
Member Author

Stypox commented Jun 27, 2020

What about replacing the current on-off option in settings with a spinner containing "All", "Only remote", "Only local", "None"?

@TobiGr
Copy link
Member

TobiGr commented Jun 27, 2020

Sounds good

@opusforlife2
Copy link
Collaborator

What about replacing the current on-off option in settings with a spinner containing "All", "Only remote", "Only local", "None"?

I just checked to see what a spinner is. Wouldn't it be weird to have one odd spinner menu standing out when the rest of the settings all contain pop up dialogues?

@Stypox Stypox changed the title Show local search suggestions even when (remote) search suggestions are disabled Allow choosing which types of search suggestions to show Jun 28, 2020
@Stypox
Copy link
Member Author

Stypox commented Jun 28, 2020

@TobiGr done & updated the PR description
@opusforlife2 see screenshot

@opusforlife2
Copy link
Collaborator

That's a spinner? The Android documentation is misleading, then. It looked like a Kitkat option selector, there.

@Stypox
Copy link
Member Author

Stypox commented Jun 30, 2020

@opusforlife2 no, that's not a spinner, that's a multiselect menu, I said spinner just to show that I wanted to make the menu entry contain multiple things (probably also because no other name came to my mind ;-) )

@wb9688 wb9688 modified the milestones: 0.19.6, 0.20.0 Jul 14, 2020
@wb9688 wb9688 modified the milestones: 0.20.0, 0.20.1 Jul 29, 2020
@avently
Copy link
Contributor

avently commented Sep 17, 2020

I found this PR in #3540, have the same problem. What's the state of the PR? Is something stops from merging?

@Stypox
Copy link
Member Author

Stypox commented Sep 17, 2020

This PR is blocked by the settings migration code, as explained by @TobiGr above, so his pr needs to be merged before

@Stypox Stypox removed this from the 0.20.1 milestone Oct 4, 2020
@Stypox Stypox force-pushed the search-history branch 2 times, most recently from 1e8e45b to 91b3714 Compare October 4, 2020 20:14
@Stypox
Copy link
Member Author

Stypox commented Oct 4, 2020

Rebased and added settings migration. Here are the test apks; before.apk is from dev, and after.apk is from this PR; try to update from before.apk to after.apk after having changed the search suggestions setting.
debug.zip

@Stypox Stypox requested a review from TobiGr October 4, 2020 20:22
@TobiGr TobiGr force-pushed the dev branch 2 times, most recently from 679bc75 to 2aeccc0 Compare March 16, 2021 08:24
@Stypox
Copy link
Member Author

Stypox commented Mar 17, 2021 •
edited
Loading

@TobiGr @B0pol I rebased again. As always, here are the test apks; before.apk is from dev, and after.apk is from this PR; try to update from before.apk to after.apk after having changed the search suggestions setting. debug.zip it doesn't work

@Stypox
Copy link
Member Author

Stypox commented Mar 17, 2021

  • First run migrations, then call the various setDefaultValues, since the latter requires the correct types. There was a crash with the "Content settings"' setDefaultValues call, on the search suggestion setting, since the boolean type, used on dev, does not match the new type of the (default) value, that is StringSet. Running the migrations before setting default values makes sense, so that's what I did.
  • Show a snackbar for a suggestion error instead of the error panel.
  • Do not show any error for interrupted I/O. Interrupted I/Os are expected since when the user inputs something new the previous suggestion fetch is interrupted.

I tested updating in the three ways possible (suggestions enabled/disabled in the before app and no before app installed at all) and for all of them I tried the 4 combinations of the search suggestions setting, everything worked fine.
As always, here are the test apks; before.apk is from dev, and after.apk is from this PR; you could change the search suggestions setting, then try to update from before.apk to after.apk.
debug.zip

TobiGr
TobiGr previously approved these changes Mar 17, 2021
View reviewed changes
Copy link
Member

@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.

Code looks good.

app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java Outdated Show resolved Hide resolved
@Stypox
Copy link
Member Author

Stypox commented Mar 17, 2021

@TobiGr done in a9a11f9

@TobiGr TobiGr modified the milestone: 21 Mar 17, 2021
@ShareASmile
Copy link
Collaborator

Is something that stops it from being merged?

@Stypox
Copy link
Member Author

Stypox commented Jun 15, 2021

I rebased and added a comment to explain how to add a migration to SettingMigrations.java, @TobiGr please merge if you think it is ready

@Stypox
Copy link
Member Author

Stypox commented Aug 24, 2021

I rebased again

@TobiGr
Copy link
Member

TobiGr commented Aug 24, 2021

Damn, how many times did i forget about this PR? Sorry

@TobiGr TobiGr merged commit a6d6ed6 into TeamNewPipe:dev Aug 24, 2021
This was referenced Sep 5, 2021
Merged
Closed
@Stypox Stypox deleted the search-history branch August 4, 2022 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wb9688 wb9688 wb9688 left review comments

@TobiGr TobiGr TobiGr approved these changes

Assignees
No one assigned
Labels
feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show search history even when search suggestions are disabled
6 participants
@Stypox @TobiGr @opusforlife2 @avently @ShareASmile @wb9688