Add Show Channel Details where it's missing #6919

ktprograms · 2021-08-14T01:50:44Z

What is it?

Bugfix (user facing)
Feature (user facing)
Codebase improvement (dev facing)
Meta improvement to the project (dev facing)

Description of the changes in your PR

Add Show Channel Details to Local Playlists, Watch History and Subscription Feeds
Had to update StreamEntity to add a uploader_url column. Also added a database Migration and changed the version number to 4.
If you click Show Channel Details and the uploader_url is empty in the database, it fetches it using the Newpipe Extractor and saves it to the database.
Had to add kapt "org.xerial:sqlite-jdbc:3.34.0" to build.gradle as a workaround on M1 Macs. Doesn't seem to hurt the build on other machines so I left it in.

Before/After Screenshots/Screen Record

Before:

- After:

Fixes the following issue(s)

Fixes No "show channel details" in long-press menu in feed or local playlist items #6155

TODO:

Show some kind of loading indicator while loading the uploader_url from the Newpipe Extractor.
Fix AppDatabaseTest (My SO question)

Options

Option 1: Just test migrating directly from `2` to `4`.
Option 2: Use SupportSQLiteDatabase query and DatabaseUtils.dumpCusrorToString.
Option 3: Hopefully someone knows the correct way to do it...

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.

ktprograms · 2021-08-14T04:35:04Z

Need to test:

Install the APK from GitHub Actions, then test Show Channel Details in:

Local Playlists
Watched History
Subscription Feed

Open Android Studio and connect to the app process, open newpipe.db and double click the streams table. Find the uploader_url column and delete the channel link for some videos. Then confirm that Show Channel Details still works (will take longer since it needs to call the Newpipe Extractor).

app/src/main/java/org/schabi/newpipe/database/Migrations.java

app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java

app/build.gradle

app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java

app/src/main/java/org/schabi/newpipe/database/playlist/PlaylistStreamEntry.kt

XiangRongLin · 2021-08-14T12:07:41Z

Fix AppDatabaseTest (My SO question)

For me that test has served it's purpose and can be deleted. An existing migration should never be changed afterwards =>code never changes => regression can never occur => tests are useless

Opinions? @Stypox

ktprograms · 2021-08-16T00:12:01Z

When creating a StreamEntity from a PlayQueueItem, previously it would just save an empty string. I've edited PlayQueueItem to have the uploader url and save it to the StreamEntity.

Stypox · 2021-08-16T12:27:09Z

For me that test has served it's purpose and can be deleted. An existing migration should never be changed afterwards =>code never changes => regression can never occur => tests are useless

Mmmh, you are right. Though, we need to put a comment in the database migrations file explaining that a test must be created when creating a PR with a migration, and then be removed just before the PR is merged. Another option instead would be to always keep a migration test from the current version - 1 to the current version, so that is can be used as reference, can be easily edited and does not need to be created and then removed just afterwards (which may cause problems if e.g. something is changed after it is removed and reviewers do not notice).

ktprograms · 2021-08-16T12:30:37Z

Seems like it would be a good idea to have the most recent migration test kept as a reminder / make sure there haven't been any database changes. I think any change in the database schema would make the test fail.

ktprograms · 2021-08-16T13:13:41Z

Bare minimum passing test in 02aa6fc, feel free to request changes.

XiangRongLin · 2021-08-16T15:54:26Z

Though, we need to put a comment in the database migrations file explaining that a test must be created when creating a PR with a migration, and then be removed just before the PR is merged

In a better world, you would not test the migrations, but test if the app is still working after applying the migrations.

In this case I don't see any benefit to add a test for the migration. If the feature works by trying it out manually, then the migration already succeeded. And it is simple enough, that having that manual try out is good enough.

Stypox · 2021-08-16T20:31:31Z

Ok, I get your point @XiangRongLin. Let's remove the database test completely and test migrations manually from now on then. @ktprograms sorry for having you write a bare minimum test if it is going to be removed ;-). Could you add a comment just after public final class Migrations { that goes something along the lines of:

// Test new migrations manually by importing a database from daily usage and checking if the migration works
// If you add a migration point it out in the pull request, so that others remember to test it themselves

Thank you :-)

ktprograms · 2021-08-17T00:58:06Z

@ktprograms sorry for having you write a bare minimum test if it is going to be removed ;-)

No problem. Wasn't much trouble.

Changes made in 967bdf8

app/build.gradle

Stypox · 2021-08-17T07:51:27Z

app/src/main/java/org/schabi/newpipe/database/Migrations.java

+    /////////////////////////////////////////////////////////////////////////////
+    //  Test new migrations manually by importing a database from daily usage  //
+    //  and checking if the migration works (Use the Database Inspector        //
+    //  https://developer.android.com/studio/inspect/database).                //
+    //  If you add a migration point it out in the pull request, so that       //
+    //  others remember to test it themselves.                                 //
+    /////////////////////////////////////////////////////////////////////////////


Perfect, thank you :-)

Stypox · 2021-08-17T08:00:14Z

app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java

+        if (isNullOrEmpty(item.getUploaderUrl())) {
+            final int serviceId = item.getServiceId();
+            final String url = item.getUrl();
+            // TODO: Some visual loading indicator


I don't know what to suggest; maybe it can do without a loading indicator (similar to the "Mark as watched" action present in 0.21.9).

I think there should be some kind of indicator (even just a Toast), won't seem like the app's hanging.

app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java

ktprograms · 2021-08-21T01:36:38Z

How can I comment on lines of code without doing a review?

opusforlife2 · 2021-08-21T16:58:10Z

@ktprograms There is a review type "Comment" meant just for this. You can select that, or choose to "Add a single comment" directly.

ktprograms · 2021-08-22T00:30:40Z

There is a review type "Comment" meant just for this. You can select that, or choose to "Add a single comment" directly.

Thanks, I clicked the "Add single comment" button so why does it show as ktprograms reviewed xx hours ago?

ktprograms · 2021-08-22T09:59:22Z

@Stypox do you mind trying to import again (maybe from a different export) since this issue can't seem to be reproduced?

opusforlife2 · 2021-08-22T16:09:34Z

@ktprograms Because when you're commenting on the code you're also reviewing it, in essence? Does it matter, though? It's just semantics. The functionality is there for you to comment on code. How Github shows it or what it's called doesn't seem important to me.

Stypox

My database was missing a CREATE INDEX `index_feed_group_sort_order` ON `feed_group` (`sort_order`);}, probably since I used that database in a temporary version of #2309 and then exported and re-imported it in a normal NewPipe release, and the correct migrations never ran since the database version was already up to date. So yes, ignore my crash report. I can confirm this works, thank you!

Stypox · 2021-08-24T13:12:35Z

Uhm, the build is failing @ktprograms

ktprograms · 2021-08-24T13:14:36Z

Not sure why, seems to be a checkstyle error but it doesn't happen on my local repo.

ktprograms · 2021-08-24T13:17:42Z

Oops, was testing building on the wrong branch.

@Stypox can you re-run the build please?

Stypox

Thank you!

app/build.gradle

Stypox

Thank you!

This checks if the uploaderUrl is in the database, if not it gets the uploaderUrl and puts it in the database. This is similar to the fetching of uploaderUrl when it doesn't exist done in TeamNewPipe#6919.

This checks if the uploaderUrl is in the database, if not it gets the uploaderUrl and puts it in the database. This is similar to the fetching of uploaderUrl when it doesn't exist done in TeamNewPipe#6919. Also use createNotification when error occurs in getStreamInfo.

ktprograms added 4 commits August 13, 2021 16:44

Add uploader_url column to StreamEntity

759a078

Add Show Channel Details to Subscription Feed & History

c248741

Save channelUrl to Database if it doesn't exist

76803bf

Catch error from ExtractorHelper.getStreamInfo, remove blockingGet

d8888e3

ktprograms changed the title ~~Channel details all places~~ Add Show Channel Details where it's missing Aug 14, 2021

XiangRongLin reviewed Aug 14, 2021

View reviewed changes

ktprograms added 3 commits August 14, 2021 15:38

Remove hardcoded string

241054f

Refactor duplicated code into method

bd0427c

Make uploader_url column nullable

21bcade

ktprograms requested a review from XiangRongLin August 14, 2021 09:50

XiangRongLin reviewed Aug 14, 2021

View reviewed changes

app/src/main/java/org/schabi/newpipe/database/playlist/PlaylistStreamEntry.kt Outdated Show resolved Hide resolved

XiangRongLin mentioned this pull request Aug 14, 2021

Extract uploader information into seperate DB table out of stream table #6924

Open

3 tasks

ktprograms added 3 commits August 14, 2021 20:54

Add comment about xerial sqlite workaround

65f2730

Fix typo in app/build.gradle

85fbd25

Remove setting uploaderUrl to empty string if null

6f1958d

triallax added the feature request Issue is related to a feature in the app label Aug 14, 2021

Save uploader url when adding from PlayQueueItem

712985c

This comment has been minimized.

Sign in to view

Remove v2 to v3 migration test, add v3 to v4 test

02aa6fc

Remove migration test, add manual testing reminder to Migrations.java

967bdf8

Stypox requested changes Aug 17, 2021

View reviewed changes

Add Loading Channel Details Toast

a209e87

ktprograms dismissed Stypox’s stale review via a209e87 August 21, 2021 01:30

ktprograms commented Aug 21, 2021

View reviewed changes

app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java Show resolved Hide resolved

Stypox previously approved these changes Aug 24, 2021

View reviewed changes

Fix checkstyle ParenPad error

2928df0

ktprograms dismissed Stypox’s stale review via 2928df0 August 24, 2021 13:17

Stypox requested changes Aug 24, 2021

View reviewed changes

app/build.gradle Outdated Show resolved Hide resolved

Remove xerial sqlite dependency

7e27e73

Stypox approved these changes Aug 24, 2021

View reviewed changes

Stypox merged commit 2027b74 into TeamNewPipe:dev Aug 24, 2021

ktprograms deleted the channel-details-all-places branch August 25, 2021 00:37

ktprograms restored the channel-details-all-places branch August 25, 2021 00:39

This was referenced Sep 5, 2021

Release 0.21.10 #7063

Merged

Release 0.21.10 #7066

Closed

ktprograms mentioned this pull request Dec 13, 2021

Load uploaderUrl when showing Channel Details from Play Queue #7538

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Show Channel Details where it's missing #6919

Add Show Channel Details where it's missing #6919

ktprograms commented Aug 14, 2021

ktprograms commented Aug 14, 2021

XiangRongLin commented Aug 14, 2021

ktprograms commented Aug 16, 2021

Stypox commented Aug 16, 2021

ktprograms commented Aug 16, 2021

This comment has been minimized.

ktprograms commented Aug 16, 2021

XiangRongLin commented Aug 16, 2021

Stypox commented Aug 16, 2021

ktprograms commented Aug 17, 2021 •

edited

Loading

Stypox Aug 17, 2021

Stypox Aug 17, 2021

ktprograms Aug 17, 2021 •

edited

Loading

ktprograms commented Aug 21, 2021

opusforlife2 commented Aug 21, 2021

ktprograms commented Aug 22, 2021

ktprograms commented Aug 22, 2021

opusforlife2 commented Aug 22, 2021

Stypox left a comment

Stypox commented Aug 24, 2021

ktprograms commented Aug 24, 2021

ktprograms commented Aug 24, 2021

Stypox left a comment

Stypox left a comment

Add Show Channel Details where it's missing #6919

Add Show Channel Details where it's missing #6919

Conversation

ktprograms commented Aug 14, 2021

What is it?

Description of the changes in your PR

Before/After Screenshots/Screen Record

Fixes the following issue(s)

TODO:

APK testing

Due diligence

ktprograms commented Aug 14, 2021

Need to test:

XiangRongLin commented Aug 14, 2021

ktprograms commented Aug 16, 2021

Stypox commented Aug 16, 2021

ktprograms commented Aug 16, 2021

This comment has been minimized.

ktprograms commented Aug 16, 2021

XiangRongLin commented Aug 16, 2021

Stypox commented Aug 16, 2021

ktprograms commented Aug 17, 2021 • edited Loading

Stypox Aug 17, 2021

Choose a reason for hiding this comment

Stypox Aug 17, 2021

Choose a reason for hiding this comment

ktprograms Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

ktprograms commented Aug 21, 2021

opusforlife2 commented Aug 21, 2021

ktprograms commented Aug 22, 2021

ktprograms commented Aug 22, 2021

opusforlife2 commented Aug 22, 2021

Stypox left a comment

Choose a reason for hiding this comment

Stypox commented Aug 24, 2021

ktprograms commented Aug 24, 2021

ktprograms commented Aug 24, 2021

Stypox left a comment

Choose a reason for hiding this comment

Stypox left a comment

Choose a reason for hiding this comment

ktprograms commented Aug 17, 2021 •

edited

Loading

ktprograms Aug 17, 2021 •

edited

Loading