Fix "Add to playlist" not working and cleanup "RouterActivity" choice handling #8340

litetex · 2022-05-02T20:38:02Z

What is it?

Bugfix (user facing)

Description of the changes in your PR

Problem: Setting Preferred 'open' action to "Add to playlist" does nothing when opening a video
Discovered while working on #8332

This PR also cleans up how the choices are handled inside RouterActivity. It was until now possible to e.g. download a channel (which resulted in an error/unsupported url page) due to incorrect choice-availability-checks.
Now everything is centralized in open place and should be easier to maintain.

Before/After Screenshots/Screen Record

Before: Nothing happens when clicking a video; Behaves like "Show info"
After:

NPRouterActivityDemo1.gif.mp4

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

I read the contribution guidelines.

Stypox

Code mostly looks good

Stypox · 2022-05-23T07:20:51Z

app/src/main/java/org/schabi/newpipe/RouterActivity.java


+        final List<AdapterChoiceItem> returnedItems = new ArrayList<>();
+        returnedItems.add(showInfo); // Always present


Uhm, why is it always present? Shouldn't it be hidden in cases where it would do the exact same thing as videoPlayer, to save some space?

You would end up with an action "Video player" that behaves like "Show Info". If thought logically it should be exactly opposite: We shouldn't show "Video player" when it can be done with "Show info".

This would require a ton of if logic and another logical layer on top of the choices. Which is in my opinion overkill.

It may be unexpected for the user if no "Show info" or "Video player" is present.

Saving space is not relevant because there is still a huge portion of users that have all entries anyway visible (e.g. if the have auto-play enabled - which is default).

Mmmh ok, then in a separate PR the router activity should be made scrollable. When I have the phone in landscape I can't see all options.

Oh, I didn't notice that.
~~Sound's like a new issue 😉~~

Update: Fixed it on the fly.
The cause of the problem was single_choice_dialog_view:

The view is used inside:

The RouterActivity

The PlayerCrasher-Dialog

The Player-Notfication configurator:

All those are now scrollable.

And the Player-Notfication configurator also uses viewbinding now.

If this is too much for the PR I can also create a separate PR.

app/src/main/java/org/schabi/newpipe/RouterActivity.java

Stypox · 2022-05-23T07:24:35Z

app/src/main/java/org/schabi/newpipe/RouterActivity.java

+            final List<StreamingService.ServiceInfo.MediaCapability> capabilities =
+                    currentService.getServiceInfo().getMediaCapabilities();


Uhm, why isn't checkstyle complaining? The = should be on the newline

Google (Example), ~~Sun~~Oracle (Example), Checkstyles default style and Spring (Example) do it on the same line and break after the assignment if necessary.
I think above is more or less industry standard, so why move the = to a new line?

Actually, from the checkstyle link you provided, the default value is nl, i.e. putting the operator on new line. It always has been this way in NewPipe, so I think you should stick to that (as you did e.g. for && and || in ifs), though this is really minor.

Actually, from the checkstyle link you provided, the default value is nl, i.e. putting the operator on new line.

Uhm no:

It only checks the tokens in the right (not left) column.

It always has been this way in NewPipe...

Also no. Example:

NewPipe/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java

Lines 138 to 147 in 1daece3

public static final String ACTION_SHOW_MAIN_PLAYER =

App.PACKAGE_NAME + ".VideoDetailFragment.ACTION_SHOW_MAIN_PLAYER";

public static final String ACTION_HIDE_MAIN_PLAYER =

App.PACKAGE_NAME + ".VideoDetailFragment.ACTION_HIDE_MAIN_PLAYER";

public static final String ACTION_PLAYER_STARTED =

App.PACKAGE_NAME + ".VideoDetailFragment.ACTION_PLAYER_STARTED";

public static final String ACTION_VIDEO_FRAGMENT_RESUMED =

App.PACKAGE_NAME + ".VideoDetailFragment.ACTION_VIDEO_FRAGMENT_RESUMED";

public static final String ACTION_VIDEO_FRAGMENT_STOPPED =

App.PACKAGE_NAME + ".VideoDetailFragment.ACTION_VIDEO_FRAGMENT_STOPPED";

or

NewPipe/app/src/main/java/org/schabi/newpipe/download/DownloadDialog.java

Lines 131 to 139 in 1daece3

private final ActivityResultLauncher<Intent> requestDownloadSaveAsLauncher =

registerForActivityResult(

new StartActivityForResult(), this::requestDownloadSaveAsResult);

private final ActivityResultLauncher<Intent> requestDownloadPickAudioFolderLauncher =

registerForActivityResult(

new StartActivityForResult(), this::requestDownloadPickAudioFolderResult);

private final ActivityResultLauncher<Intent> requestDownloadPickVideoFolderLauncher =

registerForActivityResult(

new StartActivityForResult(), this::requestDownloadPickVideoFolderResult);

Update: I went a bit through _git history and found these commits:

Check code style with Checkstyle #3282 - fda5405 (looks like the origin of the = on new line)

Expandable player with unified UI #2907 - d8f7db4 (overwrites/reverts some of these changes)

So apparently someone (cough wb9688) introduced his personal code "style" but didn't persist the changes inside checkstyle and also no one noticed it inside the PR...

Anyway I - and I also think most other developers (as seen above) - favor the = after assignment style. So I would stick with that ;)

Mmmh ok. I still don't see why all operators should be on newlines (like you did for || below here) but = should not, but anyway... If this is the standard across the industry, if you have a little spare time please open a PR to enforce such style in checkstyle

Stypox · 2022-05-23T07:24:47Z

app/src/main/java/org/schabi/newpipe/RouterActivity.java

-                serviceSupportsChoice = capabilities.contains(AUDIO);
+            // Check if the service supports the choice
+            if (isVideoPlayerSelected && capabilities.contains(VIDEO)
+                    || isAudioPlayerSelected && capabilities.contains(AUDIO)) {


Parenthesis around the &&s

…nding

Stypox

Thank you! Code looks good to me. I also tested and everything works well.

A note for a separate PR: the "processing, will take a moment" toast should be shown on any action that waits for video info to be obtained. Currently it is only shown for "Add to playlist". I think the same string should also be used for actions in the long-press menu.

Stypox

Wait, when the router activity closes (because the dialog is dismissed), a strange animation is shown with a red line swiping through the screen. I thought this happened on dev, too, but it doesn't. I think it is some activity theming issue.

litetex · 2022-06-04T13:01:52Z

Wait, when the router activity closes (because the dialog is dismissed), a strange animation is shown with a red line swiping through the screen. I thought this happened on dev, too, but it doesn't. I think it is some activity theming issue.

I think you mean this or?

grafik](https://user-images.githubusercontent.com/40789489/172000149-c3ccac1b-cf1e-4256-b8da-92285759fd35.png) ![NPRouterActivityP1

I will have a look into it...

litetex · 2022-06-04T13:21:18Z

Look a easy fix:
https://stackoverflow.com/a/26927698

Oh we are still supporting Android 4.4.... So not an easy fix... yay

Fixed it only for Android 5+ up because Android 4.4 will soon be no longer supported.

And btw, this issue arose in #8332. So a new PR would have been better :)

Fix for TeamNewPipe#8332

sonarqubecloud · 2022-06-04T13:31:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

Stypox

Great, thank you :-)

litetex added the bug Issue is related to a bug label May 2, 2022

opusforlife2 changed the title ~~Fix "add to playlist" not working and cleanup RouterActicity choice handling~~ Fix "Add to playlist" not working, and cleanup "RouterActivity" choice handling May 2, 2022

This comment was marked as off-topic.

Sign in to view

litetex marked this pull request as ready for review May 3, 2022 17:19

litetex changed the title ~~Fix "Add to playlist" not working, and cleanup "RouterActivity" choice handling~~ Fix "Add to playlist" not working and cleanup "RouterActivity" choice handling May 3, 2022

This comment was marked as off-topic.

Sign in to view

litetex mentioned this pull request May 22, 2022

fix button cannot be seen when download popup file already exists #8305

Closed

5 tasks

Stypox requested changes May 23, 2022

View reviewed changes

litetex added 2 commits May 28, 2022 00:39

Fix add to playlist

733f6aa

Reworked incorrect choice handling and centralized it

a68c6a2

litetex force-pushed the fix-add-to-playlist branch from c492d1f to 1f5795d Compare May 27, 2022 22:45

litetex added 3 commits May 28, 2022 00:46

Improved docs, format and code style

d2967f5

Code cleanup

911ac65

Bonus fix: Made single_choice_dialog_view scrollable + use viewbi…

2985258

…nding

litetex force-pushed the fix-add-to-playlist branch from 1f5795d to 2985258 Compare May 27, 2022 22:47

Stypox approved these changes May 30, 2022

View reviewed changes

Stypox requested changes May 30, 2022

View reviewed changes

Made statusbar color transparent for RouterActivity (Android 5+)

be5af0b

Fix for TeamNewPipe#8332

Stypox approved these changes Jun 5, 2022

View reviewed changes

Stypox merged commit a59660f into TeamNewPipe:dev Jun 5, 2022

litetex deleted the fix-add-to-playlist branch June 10, 2022 12:59

Stypox mentioned this pull request Jun 24, 2022

Release v0.23.1 (please TEST!) #8548

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "Add to playlist" not working and cleanup "RouterActivity" choice handling #8340

Fix "Add to playlist" not working and cleanup "RouterActivity" choice handling #8340

litetex commented May 2, 2022 •

edited

Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

Stypox left a comment

Stypox May 23, 2022

litetex May 23, 2022

Stypox May 24, 2022

litetex May 27, 2022 •

edited

Loading

Stypox May 23, 2022

litetex May 23, 2022

Stypox May 24, 2022

litetex May 27, 2022

Stypox May 30, 2022

Stypox May 23, 2022 •

edited

Loading

Stypox left a comment •

edited

Loading

Stypox left a comment

litetex commented Jun 4, 2022 •

edited by github-actions bot

Loading

litetex commented Jun 4, 2022

sonarqubecloud bot commented Jun 4, 2022

Stypox left a comment


		final List<AdapterChoiceItem> returnedItems = new ArrayList<>();
		returnedItems.add(showInfo); // Always present

		final List<StreamingService.ServiceInfo.MediaCapability> capabilities =
		currentService.getServiceInfo().getMediaCapabilities();

	public static final String ACTION_SHOW_MAIN_PLAYER =
	App.PACKAGE_NAME + ".VideoDetailFragment.ACTION_SHOW_MAIN_PLAYER";
	public static final String ACTION_HIDE_MAIN_PLAYER =
	App.PACKAGE_NAME + ".VideoDetailFragment.ACTION_HIDE_MAIN_PLAYER";
	public static final String ACTION_PLAYER_STARTED =
	App.PACKAGE_NAME + ".VideoDetailFragment.ACTION_PLAYER_STARTED";
	public static final String ACTION_VIDEO_FRAGMENT_RESUMED =
	App.PACKAGE_NAME + ".VideoDetailFragment.ACTION_VIDEO_FRAGMENT_RESUMED";
	public static final String ACTION_VIDEO_FRAGMENT_STOPPED =
	App.PACKAGE_NAME + ".VideoDetailFragment.ACTION_VIDEO_FRAGMENT_STOPPED";

	private final ActivityResultLauncher<Intent> requestDownloadSaveAsLauncher =
	registerForActivityResult(
	new StartActivityForResult(), this::requestDownloadSaveAsResult);
	private final ActivityResultLauncher<Intent> requestDownloadPickAudioFolderLauncher =
	registerForActivityResult(
	new StartActivityForResult(), this::requestDownloadPickAudioFolderResult);
	private final ActivityResultLauncher<Intent> requestDownloadPickVideoFolderLauncher =
	registerForActivityResult(
	new StartActivityForResult(), this::requestDownloadPickVideoFolderResult);

Fix "Add to playlist" not working and cleanup "RouterActivity" choice handling #8340

Fix "Add to playlist" not working and cleanup "RouterActivity" choice handling #8340

Conversation

litetex commented May 2, 2022 • edited Loading

What is it?

Description of the changes in your PR

Before/After Screenshots/Screen Record

APK testing

Due diligence

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

Stypox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

litetex May 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stypox May 23, 2022 • edited Loading

Choose a reason for hiding this comment

Stypox left a comment • edited Loading

Choose a reason for hiding this comment

Stypox left a comment

Choose a reason for hiding this comment

litetex commented Jun 4, 2022 • edited by github-actions bot Loading

litetex commented Jun 4, 2022

sonarqubecloud bot commented Jun 4, 2022

Stypox left a comment

Choose a reason for hiding this comment

litetex commented May 2, 2022 •

edited

Loading

litetex May 27, 2022 •

edited

Loading

Stypox May 23, 2022 •

edited

Loading

Stypox left a comment •

edited

Loading

litetex commented Jun 4, 2022 •

edited by github-actions bot

Loading