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

Display search suggestion: did you mean & showing result for #3471

Merged
merged 6 commits into from
Jul 7, 2020
Merged

Display search suggestion: did you mean & showing result for #3471

merged 6 commits into from
Jul 7, 2020

Conversation

Royosef
Copy link
Contributor

@Royosef Royosef commented Apr 22, 2020 •
edited by Stypox
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

Display search suggestions: "did you mean" and "showing result for" (in YouTube & YouTube Music).

Fixes the following issue(s)

Relies on the following changes

Testing apk

NewPipe_gisplaysearchsuggestion-debug.zip

Agreement

@wb9688 wb9688 added this to the 0.19.4 milestone Apr 22, 2020
@wb9688
Copy link
Contributor

wb9688 commented Apr 22, 2020

You haven't changed app/build.gradle to have implementation 'com.github.B0pol:NewPipeExtractor:621be5ba3b7d72e486111555a9a0e4400c8beedd'.

wb9688
wb9688 reviewed Apr 22, 2020
View reviewed changes
app/src/main/res/layout/fragment_search.xml Outdated Show resolved Hide resolved
app/src/main/res/values/dimens.xml Outdated Show resolved Hide resolved
B0pol
B0pol requested changes Apr 22, 2020
View reviewed changes
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@Royosef
Copy link
Contributor Author

Royosef commented Apr 22, 2020

You haven't changed app/build.gradle to have implementation 'com.github.B0pol:NewPipeExtractor:621be5ba3b7d72e486111555a9a0e4400c8beedd'.

Oh, I did. It's just I wasn't sure I'm supposed to include it in the PR, I will add it.

@wb9688 wb9688 added the feature request Issue is related to a feature in the app label May 9, 2020
@B0pol B0pol linked an issue May 9, 2020 that may be closed by this pull request
Search suggestions #3419
Closed
@wb9688 wb9688 force-pushed the DisplaySearchSuggestion branch 2 times, most recently from cb4da91 to 7714b8e Compare May 12, 2020 07:52
wb9688
wb9688 requested changes May 12, 2020
View reviewed changes
Copy link
Contributor

@wb9688 wb9688 left a comment

Choose a reason for hiding this comment

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

When I go back to search from a video, it won't display the search suggestion anymore.

Btw I rebased this and fixed a NullPointerException.

@Stypox Stypox modified the milestones: 0.19.4, 0.19.5 May 28, 2020
@wb9688 wb9688 force-pushed the DisplaySearchSuggestion branch 2 times, most recently from 8403ca9 to 5c8d1bf Compare June 14, 2020 08:06
wb9688
wb9688 previously approved these changes Jun 14, 2020
View reviewed changes
@wb9688 wb9688 requested a review from B0pol June 15, 2020 13:18
@wb9688 wb9688 requested a review from B0pol June 28, 2020 13:16
@B0pol
Copy link
Member

B0pol commented Jul 3, 2020

I rebased and used a switch preference instead of "follow … (theme 1)" "follow … (theme 2)"

@Stypox
Copy link
Member

Stypox commented Jul 3, 2020

@B0pol I think you picked the wrong PR ;-D
What you meant was #3632, wasn't it?

@Stypox Stypox mentioned this pull request Jul 3, 2020
Merged
5 tasks
@B0pol
Copy link
Member

B0pol commented Jul 3, 2020

Yes 🤦‍♂️

@Stypox Stypox self-assigned this Jul 4, 2020
@Stypox Stypox force-pushed the DisplaySearchSuggestion branch 2 times, most recently from cce44a1 to a1b6746 Compare July 4, 2020 22:50
@Stypox
Copy link
Member

Stypox commented Jul 4, 2020

I rebased, applied the suggestions above, and changed commit names to a better format.
I added a selectable item background to the suggestion panel, and I also made it long clickable: on a long click the current search query is replaced with the suggested one and the keyboard is opened, but the search is not performed, to let the user edit the query before searching.

Test apk: app-debug.zip

@wb9688
Copy link
Contributor

wb9688 commented Jul 5, 2020

I added a selectable item background to the suggestion panel

@Stypox: What do you mean?

on a long click the current search query is replaced with the suggested one and the keyboard is opened

It doesn't open the keyboard for me. It also would be nice if it'd have the cursor at the end of the query instead of the start.

@Stypox Stypox force-pushed the DisplaySearchSuggestion branch from a1b6746 to cce9904 Compare July 5, 2020 19:59
@Stypox Stypox force-pushed the DisplaySearchSuggestion branch from cce9904 to 13a0d1d Compare July 5, 2020 20:02
@Stypox
Copy link
Member

Stypox commented Jul 5, 2020

I rebased again and fixed the problems reported by @wb9688

What do you mean?

The background that makes bubbles around touches: ?attr:selectableItemBackground

Another testing apk: app-debug.zip

@wb9688
Copy link
Contributor

wb9688 commented Jul 5, 2020

@Stypox: Ah, you mean ripple?

@TobiGr
Copy link
Contributor

TobiGr commented Jul 7, 2020

Looks good to me. Btw. is there a reason, why the bold and italic font effects were removed from the search terms?

@B0pol
Copy link
Member

B0pol commented Jul 7, 2020

@TobiGr yes, read this thread: #3471 (comment)

@TobiGr
Copy link
Contributor

TobiGr commented Jul 7, 2020 •
edited
Loading

@B0pol Thank you. b96d171 should solve the problem. Or do you expect something else to be displayed?

@TobiGr TobiGr force-pushed the DisplaySearchSuggestion branch from acccc6e to b96d171 Compare July 7, 2020 18:23
@TobiGr TobiGr merged commit f4a4172 into TeamNewPipe:dev Jul 7, 2020
This was referenced Jul 8, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stypox Stypox Stypox left review comments

@wb9688 wb9688 wb9688 approved these changes

@B0pol B0pol Awaiting requested review from B0pol

Assignees

@Stypox Stypox

Labels
feature request Issue is related to a feature in the app
Projects
None yet
Milestone
0.19.6
Development

Successfully merging this pull request may close these issues.

Search suggestions
5 participants
@Royosef @wb9688 @B0pol @Stypox @TobiGr