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

YouTube Music search stuff #3240

Merged
merged 8 commits into from
Apr 10, 2020
Merged

YouTube Music search stuff #3240

merged 8 commits into from
Apr 10, 2020

Conversation

wb9688
Copy link
Contributor

@wb9688 wb9688 commented Mar 17, 2020

See TeamNewPipe/NewPipeExtractor#291.

@wb9688 wb9688 added feature request Issue is related to a feature in the app youtube Service, https://www.youtube.com/ labels Mar 20, 2020
@wb9688
Copy link
Contributor Author

wb9688 commented Mar 20, 2020

@opusforlife2: Here is a test APK, in case you wanted to test it, since you opened #3202.

@opusforlife2
Copy link
Collaborator

opusforlife2 commented Mar 20, 2020

Yay! I'm gonna use this so much. ಠ‿ಠ

Edit: I love you. (づ ̄ ³ ̄)づ

@opusforlife2
Copy link
Collaborator

opusforlife2 commented Mar 20, 2020

This occurs for all 4 "Music/" options

Exception

  • User Action: searched
  • Request: megaman x3
  • Content Language: en_US
  • Service: YouTube
  • Version: 0.18.7
  • OS: Linux Android 9 - 28
Crash log

java.lang.NullPointerException: Attempt to invoke virtual method 'com.grack.nanojson.JsonArray com.grack.nanojson.JsonObject.getArray(java.lang.String)' on a null object reference
	at org.schabi.newpipe.extractor.services.youtube.extractors.YoutubeSearchExtractor.getInitialPage(YoutubeSearchExtractor.java:198)
	at org.schabi.newpipe.extractor.utils.ExtractorHelper.getItemsPageOrLogError(ExtractorHelper.java:19)
	at org.schabi.newpipe.extractor.search.SearchInfo.getInfo(SearchInfo.java:50)
	at org.schabi.newpipe.extractor.search.SearchInfo.getInfo(SearchInfo.java:30)
	at org.schabi.newpipe.util.ExtractorHelper.lambda$searchFor$0(ExtractorHelper.java:82)
	at org.schabi.newpipe.util.-$$Lambda$ExtractorHelper$BBduYDeZ_vXMQYaemaggmTPtqvA.call(Unknown Source:8)
	at io.reactivex.internal.operators.single.SingleFromCallable.subscribeActual(SingleFromCallable.java:44)
	at io.reactivex.Single.subscribe(Single.java:3438)
	at io.reactivex.internal.operators.single.SingleSubscribeOn$SubscribeOnObserver.run(SingleSubscribeOn.java:89)
	at io.reactivex.Scheduler$DisposeTask.run(Scheduler.java:578)
	at io.reactivex.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:66)
	at io.reactivex.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:57)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:301)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
	at java.lang.Thread.run(Thread.java:764)


@wb9688
Copy link
Contributor Author

wb9688 commented Mar 20, 2020

@opusforlife2: Could confirm. It's caused by that "Did you mean: mega man x3" thing at the top. I'll fix it in a few minutes.

Edit: fixed and updated APK.

@opusforlife2
Copy link
Collaborator

I would like a way to stay on Youtube Music when autoplay is on. But right now there's no guarantee that the next suggested video is a YT Music audio. That was basically what I wanted when I mentioned having YT Music as a separate selectable service. The ability to restrict all calls to music.youtube.com only.

@wb9688
Copy link
Contributor Author

wb9688 commented Mar 20, 2020

@opusforlife2: Autoplay in NewPipe is YouTube autoplay. What you also want is support for YouTube Music mixes, which are also different from YouTube mixes (that will be added by TeamNewPipe/NewPipeExtractor#280). Could you open a separate issue for that? Thanks!

@opusforlife2
Copy link
Collaborator

Edit: fixed and updated APK.

Werks!

Copy link
Contributor

@mauriciocolli mauriciocolli left a comment

Choose a reason for hiding this comment

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

One thing to keep in mind is that videos on YouTube Music returns different titles from the normal version, so it will change when opening them. I guess is not a huge issue though?

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@opusforlife2
Copy link
Collaborator

Occurs for all 4 options, again.

Exception

  • User Action: searched
  • Request: ryfffhfff
  • Content Language: en_US
  • Service: YouTube
  • Version: 0.18.7
  • OS: Linux Android 9 - 28
Crash log

java.lang.NullPointerException: Attempt to invoke virtual method 'com.grack.nanojson.JsonObject com.grack.nanojson.JsonObject.getObject(java.lang.String)' on a null object reference
	at org.schabi.newpipe.extractor.services.youtube.extractors.YoutubeSearchExtractor.getSearchSuggestion(YoutubeSearchExtractor.java:182)
	at org.schabi.newpipe.extractor.search.SearchInfo.getInfo(SearchInfo.java:45)
	at org.schabi.newpipe.extractor.search.SearchInfo.getInfo(SearchInfo.java:30)
	at org.schabi.newpipe.util.ExtractorHelper.lambda$searchFor$0(ExtractorHelper.java:82)
	at org.schabi.newpipe.util.-$$Lambda$ExtractorHelper$BBduYDeZ_vXMQYaemaggmTPtqvA.call(Unknown Source:8)
	at io.reactivex.internal.operators.single.SingleFromCallable.subscribeActual(SingleFromCallable.java:44)
	at io.reactivex.Single.subscribe(Single.java:3438)
	at io.reactivex.internal.operators.single.SingleSubscribeOn$SubscribeOnObserver.run(SingleSubscribeOn.java:89)
	at io.reactivex.Scheduler$DisposeTask.run(Scheduler.java:578)
	at io.reactivex.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:66)
	at io.reactivex.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:57)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:301)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
	at java.lang.Thread.run(Thread.java:764)


@wb9688
Copy link
Contributor Author

wb9688 commented Mar 21, 2020

@opusforlife2: I don't have that issue, though I think I've fixed it.

@opusforlife2
Copy link
Collaborator

Fixed!

Suggestion: Just like you put YouTube Music above the bottom 4 filters, why not put YouTube above the top 4? It'll look consistent and clearer to the user as well.

TobiGr
TobiGr previously approved these changes Mar 23, 2020
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.

Frontend looks good to me

Comment on lines +391 to +425
if (filter.equals("music_songs")) {
MenuItem musicItem = menu.add(2,
itemId++,
0,
"YouTube Music");
musicItem.setEnabled(false);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, adding a disabled item as a header title is bad...

Though I don't know if something like that can even be implemented in a native way (closest thing would be a sub-menu), and I'm not sure if it's worth it to add custom components when this option will be improved in the near future.

Will we just leave it like this until that day?

Copy link
Contributor Author

@wb9688 wb9688 Mar 23, 2020

Choose a reason for hiding this comment

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

No, this couldn't be implemented in a native way other than submenus. I told Tobias that I didn't really like using a disabled item for that, but he seems to be fine with it until we improve the whole search filter thing some day.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, adding a disabled item as a header title is bad...

Yes. I hope that we will get proper search filter soon.

Stypox
Stypox previously approved these changes Mar 24, 2020
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.

Looks good. @TobiGr it should be merged after TeamNewPipe/NewPipeExtractor#280 and #3243 are ready

@wb9688 wb9688 dismissed stale reviews from Stypox and TobiGr via 18279b4 April 4, 2020 14:58
@wb9688
Copy link
Contributor Author

wb9688 commented Apr 4, 2020

@TobiGr: I just rebased this PR.

@TobiGr
Copy link
Contributor

TobiGr commented Apr 6, 2020

@Stypox Why should this be merged after TeamNewPipe/NewPipeExtractor#280 and #3243?

@TobiGr TobiGr added this to the 0.19.3 milestone Apr 6, 2020
@wb9688
Copy link
Contributor Author

wb9688 commented Apr 7, 2020

@TobiGr: He said that because it depends on the commits to add support for showing 100+/unlimited/unknown streams for playlists. However, I've added the relevant commits to this PR as you could see.

@Stypox
Copy link
Member

Stypox commented Apr 10, 2020

@TobiGr oh ok, then merge it, the other ones can be rebased afterwards

Stypox
Stypox previously approved these changes Apr 10, 2020
Stypox added 2 commits April 10, 2020 10:35
ITEM_COUNT_INFINITE and ITEM_COUNT_MORE_THAN_100.
Use localizeStreamCount in PlaylistFragment and PlaylistItemHolder
@wb9688 wb9688 merged commit bd9b2d5 into TeamNewPipe:dev Apr 10, 2020
This was referenced Apr 24, 2020
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 youtube Service, https://www.youtube.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants