Add settings migration, improve share dialog and minimize to background by default #4259

TobiGr · 2020-09-09T19:27:37Z

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

The difference between "detail page" and the video player is only the phone's orientation. Therefore, we decided to remove the open "Detail Page" option from the share menu. To handle entries from older versions, I added a migration system for the settings / shared preferences which is quite similar to the one used in for the DB.
With the new app workflow introduced by the unified player PR, it is possible to minimize the video player to the bottom while doing other stuff in the app. However, this does only apply to the main fragment. When a user opens the settings or the about screen, the player stops. That's bad UX. For this reason, I set the "Minimize on app switch" setting to "background" instead of "None" to allow seamless transitions within the app as well as when closing NewPipe.

Fixes the following issue(s)

Closes #4070

Testing apk

note: I did not have much time for testing.

Please install the current dev version from the zip first and change the "Minimize on app switch" setting or keep it as it is to see if the migration works when updating with the other APK.

apps.zip

Agreement

I carefully read the contribution guidelines and agree to them.

opusforlife2 · 2020-09-09T19:33:21Z

#4071 (comment)

opusforlife2 · 2020-09-09T19:38:48Z

When a user opens the settings or the about screen, the player stops.

The proper solution to this is still #3477, right?

TobiGr · 2020-09-09T19:40:05Z

yes, but that's out of scope for this version

MD77MD · 2020-09-09T21:56:30Z

could this be done for this as well #3178 (comment) this exactly the same solution i suggested.

i would suggest reading from #3178 (comment) to have a better understanding

Stypox

I tested updating from the dev apk to the other under many configurations and everything worked just fine. Thank you!

app/src/main/java/org/schabi/newpipe/settings/SettingMigrations.java

Stypox · 2020-09-10T21:02:14Z

app/src/main/java/org/schabi/newpipe/settings/SettingMigrations.java

+import org.schabi.newpipe.report.ErrorActivity.ErrorInfo;
+import org.schabi.newpipe.report.UserAction;
+
+public final class SettingMigrations {


Not in this PR, but we may consider adding junit tests for migrations, using some kind of mocked shared preferences

Completely agree with that.

Stypox · 2020-09-10T21:17:26Z

I was told of an inconsistency with the minimize on app switch options.

tap and play a video, and wait for it to start playing
press the back hardware button until the app is closed

If minimize is set to popup, a popup will start, but if set to background the app just closes. I think this should be fixed before releasing 0.20.0, it shouldn't be difficult, so I'll look into it tomorrow.

opusforlife2

Reminder regarding the autoplay discussion yesterday: #4071 (comment)

opusforlife2 · 2020-09-11T10:47:37Z

Do you intend for this to be a temporary fix for

When a user opens the settings or the about screen, the player stops. That's bad UX.

until #3477 is fixed or will this be a permanent change?

I think it might be better off as a permanent change. Minimizing is a feature Newpipe has that makes it superior to Youtube. Better that we alert the user that such a feature exists and let them turn it off manually, than to hide it and risk that most users don't even realise it is there.

I set the "Minimize on app switch" setting to "background" instead of "None" to allow seamless transitions within the app as well as when closing NewPipe.

Should there be a toast the first 2-3 times this happens? A user who is new to Newpipe (New to the Pipe? 🤭) might get surprised or think it is a bug.

Something like "Minimizing to background/popup. This can be changed in settings."

If the user does change it in settings, this toast could be turned off immediately. Otherwise, after it has been shown twice or thrice.

MD77MD · 2020-09-11T17:39:27Z

If the user does change it in settings, this toast could be turned off immediately. Otherwise, after it has been shown twice or thrice.

I would leave it until changed in settings... no need to hide it

opusforlife2 · 2020-09-11T17:48:01Z

I would leave it until changed in settings... no need to hide it

It would be very irritating to see the toast every time when you know you can change it in settings but you want to continue with the default setting. Having to change a setting, then change it back again to get rid of a toast is bad UX.

MD77MD · 2020-09-12T11:56:34Z

@opusforlife2
sorry... i thought you meant to disable background playback after three times... I agree with you on the toasta

TobiGr · 2020-09-13T15:39:42Z

Update APKs: apps.zip

I only fixed #4259 (comment)

Stypox

Thanks, the first-time check looks good now

app/src/main/java/org/schabi/newpipe/settings/SettingMigrations.java

TobiGr · 2020-09-19T11:28:52Z

Fixed a typo

Stypox

Please review lines 452 to 470 of RouterActivity.java, I think that piece of code can be deleted, as "show_info_key" is not a choice anymore.

Stypox · 2020-09-19T12:09:46Z

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

            returnList.add(new AdapterChoiceItem(getString(R.string.video_player_key),
                    getString(R.string.video_player),


In order to achieve what was discussed in #4071, I'd just add a condition here that shows "R.string.show_info" if autoplay is off, but still uses "R.string.video_player_key" as the key in any case. At the end of the day both options open the same activity, even though the outcome is different, so I think users would expect "Show info" to be selected in the dialog if they had used the "Video player" choice before disabling autoplay, and viceversa.

I don't think you would need to do anything else in order to achieve what I described above, since VideoDetailFragment already handles autoplay by itself.

opusforlife2 · 2020-09-22T15:41:03Z

When chosen, auto-start the playback, even when the device is in portrait mode and auto-play is disabled.

Can you clarify this a little bit? If I have autoplay set to off, and Preferred open action is set to Video player (after Show info is removed) then will opening links from other apps autoplay the video?

Stypox · 2020-09-22T17:49:15Z

@opusforlife2 I think commit messages are still the old ones, so don't look at them now ;-)

TobiGr · 2020-09-22T18:01:20Z

I did not change anything regarding @Stypox's suggestion yet. Last pushs were just fixes to the migration process. In the first implementations, all migrations were run and not only the ones for newer pref versions.

Stypox · 2020-09-22T21:29:32Z

Remove "Detail Page" open action from share dialog under certain circumstances
With the new application workflow and unified player, video detail page and video player are the same activity. So show only one of these options based on whether autoplay is enabled or not, and show both if using external player

New APKs with the changes quoted above: apks.zip

Please test this, everything should work fine under many conditions:

upgrading from before.apk to after.apk
importing files from old newpipe versions in after.apk
sharing videos, playlists, channels to newpipe and the correct options turn up in after.apk. In particular for videos, where the behaviour was changed, make sure the statements quoted above hold true.
@opusforlife2 is the new behaviour suitable?

opusforlife2 · 2020-09-23T05:22:40Z

before.apk is identical to the latest debug?

TobiGr

@Stypox Your changes look good 👍

opusforlife2 · 2020-09-26T00:41:26Z

When sharing to 'after.apk', you first see a notification saying "Getting info - loading requested content", and only after it is loaded does the app open. This isn't mentioned in the changes above. Also, this delay might be perceived as a bug or Newpipe being slow by the user. Maybe a toast should be shown additionally?

sharing videos, playlists, channels to newpipe and the correct options turn up in after.apk

Correct options do show up for videos, but what has changed for playlists or channels?

Depending on the autoplay setting, the Always ask menu shows 'Show info' or 'Video player'. Shouldn't this also happen for the selection menu in settings? That menu always shows 'Video player', even if autoplay is off. Could be confusing or misleading for users.

Both minimize and preferred open action settings migrations happen correctly, for upgrading as well as importing. 👍

Stypox · 2020-09-26T06:27:12Z

This isn't mentioned in the changes above.

That's always been there

Correct options do show up for videos, but what has changed for playlists or channels?

Both show info and video player should always be shown for channels and playlist, since they actually do different things

Shouldn't this also happen for the selection menu in settings?

No, since as I said above both info and player are shown for playlists and channels

Both minimize and preferred open action settings migrations happen correctly, for upgrading as well as importing. 👍

Great :-D

opusforlife2 · 2020-09-26T09:14:49Z

That's always been there

Doesn't happen for me in either 0.19.8 or the latest unified debug. Only in this apk. Release and debug first open, then load the video details. This apk first loads video details, then opens with them already showing.

Both show info and video player should always be shown for channels and playlist

Wow, before this I didn't even know it was possible to play a queue externally. :P My bad. I didn't select Always Ask before testing playlists and channels.

No, since as I said above both info and player are shown for playlists and channels

I'm not talking about the Always Ask menu. I'm talking about the Preferred open action menu with radio buttons. It doesn't have Show Info when Autoplay is off, but still shows Video Player.

In any case, the radio buttons seem to be for the case of a single video link being opened from another app. I think we might need separate Preferred 'open' action settings for playlists and channels. Because the current setting isn't geared towards them. Playlists and channels being affected seems more like a byproduct of the setting.

…umstances With the new application workflow and unified player, video detail page and video player are the same activity. So show only one of these options based on whether autoplay is enabled or not, and show both if using external player

Stypox · 2020-09-26T20:02:26Z

I think we might need separate Preferred 'open' action settings for playlists and channels

Yes

I'm talking about the Preferred open action menu with radio buttons
Doesn't happen for me in either 0.19.8 or the latest unified debug. Only in this apk. Release and debug first open, then load the video details. This apk first loads video details, then opens with them already showing.

Does this apk work as expected? I'm not providing before and after apks, since the only thing I did was restoring some things unrelated to migration app-debug.zip.

opusforlife2 · 2020-09-26T20:16:08Z

Oh no. The package is named org.schabi.newpipe.debug.pr, which is also the name of the notification PR apk. It installed over that one. xD

opusforlife2 · 2020-09-26T20:29:16Z

The Getting requested info notification is gone now, and the behaviour is as it was before. Is that what we want, though? I kind of liked the new behaviour. Didn't like the idea of a toast?

Now 'Preferred open action' shows both Show Info and Video player. That's not what I meant. When I set Autoplay to Off, I should see only Show Info. When I set it to On or Wifi, I should see only Video player. At least, that's what I thought up when I originally wrote the comment in that issue.

Stypox · 2020-09-26T20:34:02Z

@opusforlife2 that menu for now also applies to channels and playlists, so both "show info" and "video player" should be there. In a future PR we could add separated settings, and at that point I'd agree with you. But for now both have to be kept.

Stypox · 2020-09-26T20:34:29Z

Didn't like the idea of a toast?

This PR has nothing to do with that notification, so that won't be changed here

opusforlife2 · 2020-09-26T20:36:28Z

Okay. Then selecting either Show info or Video player would mean the same thing for a video link, opening the video details page, right? Whether or not the video autoplays would then depend on that setting?

opusforlife2 · 2020-09-26T20:36:44Z

This PR has nothing to do with that notification, so that won't be changed here

Where was it added? O.o

Stypox · 2020-09-26T20:43:48Z

@TobiGr do you agree with what I said above? If so, this is ready ;-)

_{@opusforlife2 the notification already exists when tapping video player or download, while show info is handled differently by opening the app directly. The change of behavior in the PR was just caused by a check being removed, which made it so that show info was the same as the others.}

Stypox · 2020-09-27T14:07:48Z

Then selecting either Show info or Video player would mean the same thing for a video link, opening the video details page, right? Whether or not the video autoplays would then depend on that setting?

Yes, it would mean the same thing. I realised this myself the other day, but since this part of code (i.e. RouterActivity) is rather crappy, it would take too much time to make this work as expected. So I'd prefer to put it off to a future PR, containing a RouterActivity refactor and different open options for channel, playlist and video.

TobiGr added this to the 0.20.0 milestone Sep 9, 2020

TobiGr requested a review from Stypox September 9, 2020 19:31

MD77MD mentioned this pull request Sep 10, 2020

Notification Improvements #3178

Merged

1 task

avently mentioned this pull request Sep 10, 2020

Video doesn't autoplay if shared to Newpipe #4071

Closed

Stypox linked an issue Sep 10, 2020 that may be closed by this pull request

Video doesn't autoplay if shared to Newpipe #4071

Closed

Stypox requested changes Sep 10, 2020

View reviewed changes

Stypox added the player Issues related to any player (main, popup and background) label Sep 10, 2020

opusforlife2 reviewed Sep 10, 2020

View reviewed changes

TobiGr force-pushed the pref_migration branch 2 times, most recently from 1801552 to f814806 Compare September 13, 2020 15:36

Stypox reviewed Sep 13, 2020

View reviewed changes

app/src/main/java/org/schabi/newpipe/settings/SettingMigrations.java Show resolved Hide resolved

TobiGr force-pushed the pref_migration branch from f814806 to 7878371 Compare September 19, 2020 11:27

Stypox requested changes Sep 19, 2020

View reviewed changes

TobiGr force-pushed the pref_migration branch 2 times, most recently from 4e46fa8 to a0c12e1 Compare September 22, 2020 13:46

Stypox force-pushed the pref_migration branch from a0c12e1 to c701d4f Compare September 22, 2020 21:17

Stypox mentioned this pull request Sep 24, 2020

Can't go directly to fullscreen when tapping thumbnail #4152

Closed

TobiGr commented Sep 25, 2020

View reviewed changes

Add migration concept for shared preferences

ad33646

Stypox force-pushed the pref_migration branch from c701d4f to 5940cc4 Compare September 26, 2020 19:48

TobiGr added 2 commits September 26, 2020 21:58

Set default value for "minimize_on_exit" to background for better UX.

3c4a4e5

Stypox force-pushed the pref_migration branch from 5940cc4 to 3c4a4e5 Compare September 26, 2020 19:58

Stypox mentioned this pull request Sep 26, 2020

Allow numbers and uppercase letters in app package id #4326

Merged

5 tasks

TobiGr merged commit d5f6033 into dev Sep 27, 2020

TobiGr changed the title ~~Add settings migration, remove "Detail page" option from share dialog and minimize to background by default~~ Add settings migration, improve share dialog and minimize to background by default Sep 27, 2020

TobiGr deleted the pref_migration branch September 27, 2020 09:21

Stypox mentioned this pull request Sep 27, 2020

Refactor open dialog with separate open preferences for videos, channels and playlists #4331

Open

This was referenced Sep 27, 2020

Release 0.20.0 #4332

Closed

Release 0.20.0 #4333

Merged

		returnList.add(new AdapterChoiceItem(getString(R.string.video_player_key),
		getString(R.string.video_player),

Add settings migration, improve share dialog and minimize to background by default #4259

Add settings migration, improve share dialog and minimize to background by default #4259

Conversation

TobiGr commented Sep 9, 2020 • edited Loading

What is it?

Description of the changes in your PR

Fixes the following issue(s)

Testing apk

note: I did not have much time for testing.

Agreement

opusforlife2 commented Sep 9, 2020

opusforlife2 commented Sep 9, 2020

TobiGr commented Sep 9, 2020

MD77MD commented Sep 9, 2020 • edited Loading

Stypox left a comment

Choose a reason for hiding this comment

Stypox Sep 10, 2020

Choose a reason for hiding this comment

TobiGr Sep 13, 2020

Choose a reason for hiding this comment

Stypox commented Sep 10, 2020 • edited Loading

opusforlife2 left a comment

Choose a reason for hiding this comment

opusforlife2 commented Sep 11, 2020 • edited by Stypox Loading

MD77MD commented Sep 11, 2020

opusforlife2 commented Sep 11, 2020

MD77MD commented Sep 12, 2020

TobiGr commented Sep 13, 2020

Stypox left a comment

Choose a reason for hiding this comment

TobiGr commented Sep 19, 2020

Stypox left a comment

Choose a reason for hiding this comment

Stypox Sep 19, 2020

Choose a reason for hiding this comment

opusforlife2 commented Sep 22, 2020

Stypox commented Sep 22, 2020

TobiGr commented Sep 22, 2020

Stypox commented Sep 22, 2020 • edited Loading

opusforlife2 commented Sep 23, 2020

TobiGr left a comment

Choose a reason for hiding this comment

opusforlife2 commented Sep 26, 2020

Stypox commented Sep 26, 2020

opusforlife2 commented Sep 26, 2020

Stypox commented Sep 26, 2020

opusforlife2 commented Sep 26, 2020

opusforlife2 commented Sep 26, 2020

Stypox commented Sep 26, 2020

Stypox commented Sep 26, 2020

opusforlife2 commented Sep 26, 2020

opusforlife2 commented Sep 26, 2020

Stypox commented Sep 26, 2020 • edited Loading

Stypox commented Sep 27, 2020

TobiGr commented Sep 9, 2020 •

edited

Loading

MD77MD commented Sep 9, 2020 •

edited

Loading

Stypox commented Sep 10, 2020 •

edited

Loading

opusforlife2 commented Sep 11, 2020 •

edited by Stypox

Loading

Stypox commented Sep 22, 2020 •

edited

Loading

Stypox commented Sep 26, 2020 •

edited

Loading