Refactor and bugfix #1623

theScrabi · 2018-08-23T12:41:38Z

This PR covers:

Fix Cyclic search result Search result only load page 2 when scroll down and keep loading infinite loop #1562
Fix Seek bar not statically lay outed
Fix YT Premium video are not blocked correctly
Fix Videos sometimes not loading (due to DASH parsing)
Fix links in video description
Show warning when someone tries to download to external sdcard
fix nothing shown exception triggers report
thumbnail not shown in background player for android 8.1 see here

karyogamy

Good work, thanks for the service player time fix.

I've only skimmed through a little bit. Will look into the download stuff some more tomorrow.

karyogamy · 2018-08-24T02:52:24Z

app/src/main/java/org/schabi/newpipe/player/BackgroundPlayer.java

@@ -309,7 +309,6 @@ private void updateNotificationThumbnail() {
 @Override
 public void onLoadingComplete(String imageUri, View view, Bitmap loadedImage) {
 super.onLoadingComplete(imageUri, view, loadedImage);
- resetNotification();


This is still required for cleaning up the existing notification (and the bitmap in it) to prevent memory leak, as described in #799. It might be a good idea to put some comments on it though.

We should eventually consider using ExoPlayer's built-in notification, which doesn't have this problem the last time I tried. However, it hardcoded the intent strings so we couldn't support multiple players concurrently. We can look into it later.

We may want to make this reset more Inteligent, on android 8+ it made the thumbnail not show up.
When we create the notification we reset it mutliple times, even after the thumbnail got set, this removes it again.

Ok I added the resetNotificateions() again, i found that the issues was that we didnt call updateNotificatinThumbnail() inside onUpdateProgress().

I think I have tried doing this too, but updateNotificatinThumbnail is very costly and can lag the phone updating bitmap every half a second. Could you try benchmarking this again, preferably on an older phone?

karyogamy · 2018-08-24T02:56:12Z

app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java

+ final List<Throwable> exceptions = result.getErrors();
+ if (!exceptions.isEmpty()
+ && !(exceptions.size() == 1
+ && exceptions.get(0) instanceof SearchExtractor.NothingFoundException)){


Maybe we should later consider removing this exception from the extractor. Is it still needed for some reason?

I thought it was including additional information. However the intention is to skip some code parts in order to jump directly to the "nothing found" routine.

Seems like bad practice if it's just there to speed up the collector process. But this is a problem for another PR, I suppose.

But this is a problem for another PR, I suppose.

I see. Another thing I thought of was that if the search result is empty, but no error is thrown, it could be an error that did not get detected. If we tell the fronted explicitly that there was nothing found, than at least we know there was no error. Maybe we can implement that without an exception, but by setting a flag.

karyogamy · 2018-08-24T03:05:53Z

app/src/main/java/org/schabi/newpipe/player/ServicePlayerActivity.java

@@ -1,8 +1,10 @@
 package org.schabi.newpipe.player;

 import android.content.ComponentName;
+import android.content.Context;


Unneeded imports?

TobiGr · 2018-08-24T13:52:25Z

app/src/main/res/values/strings.xml

@@ -168,6 +168,8 @@
 <string name="search_history_deleted">Search history deleted.</string>
 <!-- error strings -->
 <string name="general_error">Error</string>
+ <string name="download_to_sdcard_error_title">External storage not available.</string>
+ <string name="download_to_sdcard_error_message">Download to external SD Card is not possible yet. Should the download place be reseted?</string>


I think it should be Should the download place be reset? as reset behaves like set and both are irregular.

I changed it.

gregordr · 2018-08-24T18:51:04Z

I have taken a look at the link part.

Links to other videos still get shortened, like in this video: https://www.youtube.com/watch?v=ipY0bhU2fe4
I generally prefer the solution youtube has, where the link looks shortened, because descriptions look really messy now.
For non-youtube links, it looks like everything works well 👍

theScrabi · 2018-08-24T20:27:38Z

I generally prefer the solution youtube has, where the link looks shortened, because descriptions look really messy now.

True, but can the description view handle html?

gregordr · 2018-08-24T20:44:51Z

@theScrabi

I have just taken a look at my old PR, and yeah it is possible: https://streamable.com/xo0q7

karyogamy · 2018-08-25T03:55:06Z

app/src/main/java/us/shandian/giga/get/DownloadManagerImpl.java

@@ -23,7 +35,8 @@
 private static final String TAG = DownloadManagerImpl.class.getSimpleName();
 private final DownloadDataSource mDownloadDataSource;

- private final ArrayList<DownloadMission> mMissions = new ArrayList<DownloadMission>();
+ private final ArrayList<DownloadMission> mMissions = new ArrayList<>();
+ private final Context context;


This should at least be labelled as NonNull.

Ideally, it's better to pass in a listener/runnable with a function to call when an exception occurs in this case, just to avoid keeping Context in a class unless there's no better solution.

karyogamy · 2018-08-25T03:55:11Z

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

+ private static void resetDownloadFolder(SharedPreferences prefs, String key, String defaultDirectoryName) {
+ final File folder = getFolder(defaultDirectoryName);
+ SharedPreferences.Editor spEditor = prefs.edit();
+ spEditor.putString(key, new File(folder, "NewPipe").getAbsolutePath());


Might want to extract new File(folder, "NewPipe").getAbsolutePath() to a variable since it is reused a couple lines above.

bla

theScrabi · 2018-08-27T14:39:39Z

@Somethingweirdhere Could you please try to figure out what to change in order to make our textview use the href instead of what is writting in the a tag :)

gregordr · 2018-08-27T19:19:43Z

@theScrabi Sure, here you go:

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

Line 488 in 3a44e92

videoDescriptionView.setAutoLinkMask(Linkify.WEB_URLS);

This line needs to be gone, or anything in the Href will be overridden by the displayed text.

But now you have a problem: Your hrefs don't work.

<a href="/redirect?q=https%3A%2F%2Fwww.facebook.com%2FTrueDetective&v=YpUznQds8p4&event=video_description&redir_token=tDtfcIHTYhl0iYimJbvVW9I33P58MTUzNTQ4MjQ5M0AxNTM1Mzk2MDkz" class="yt-uix-sessionlink " data-url="/redirect?q=https%3A%2F%2Fwww.facebook.com%2FTrueDetective&v=YpUznQds8p4&event=video_description&redir_token=tDtfcIHTYhl0iYimJbvVW9I33P58MTUzNTQ4MjQ5M0AxNTM1Mzk2MDkz" data-sessionlink="itct=CDUQ6TgYACITCMSpypX0jd0CFU_RFQodxGAK1ij4HUie5bO70PPMymI" data-target-new-window="True" rel=" noopener" target="_blank">https://www.facebook.com/TrueDetective</a>

This is how they look with the extractor right now.

You have to make them look like this:

<a href="https://www.facebook.com/TrueDetective" class="yt-uix-sessionlink " data-sessionlink="itct=CDUQ6TgYACITCP7y8KL2jd0CFQawVQodY0gNByj4HUie5bO70PPMymI" data-target-new-window="True" data-url="/redirect?q=https%3A%2F%2Fwww.facebook.com%2FTrueDetective&event=video_description&redir_token=7xMZ9ZYoGiSxYNc0Mplf3H2aL0V8MTUzNTQ4MzA1N0AxNTM1Mzk2NjU3&v=YpUznQds8p4" target="_blank" rel=" noopener">https://www.facebook.com/TrueDetective</a>

It's what I did here, in my old PR. It works, but I agree that the solution maybe could be prettier.

theScrabi · 2018-08-28T10:18:19Z

@Somethingweirdhere I experimented with it for a while, removed that line and tried to override the href in the links, however it did not work as expected.

fixes moreacording to code review fixed link handling once more

theScrabi · 2018-08-28T10:20:58Z

@karyogamy The changes you requested should be good by now, could you please release the block.

gregordr · 2018-08-28T13:06:52Z

@theScrabi Do you think you could Upload what you tried (NewPipe and extractor) so I can take a look at it?

theScrabi · 2018-08-28T14:27:08Z

If you use this commit 3aa7ff6 from the NewPipeExctractor you can try it. This was thought to replace the href instead of the text.

So far all acomplished, the thing with the NothingFoundException can be done in ohter PR.

gregordr · 2018-08-28T17:47:15Z

@theScrabi I tried NewPipe Dev and NewPipe Extractor 3aa7ff6.

When debugging, it looks like this:

<a href="/redirect?q=https%3A%2F%2Fwww.twitter.com%2Flongbeachgriffy&v=cuL2SZmLrR0&event=video_description&redir_token=EvkYDHO1dvjA3tsBZJ0_QUymTDN8MTUzNTU2MzkzMkAxNTM1NDc3NTMy" class="yt-uix-sessionlink " data-sessionlink="itct=CDUQ6TgYACITCNuf7MajkN0CFUG3VQodU9kLsCj4HUid2q7Mmcm98XI" data-url="/redirect?q=https%3A%2F%2Fwww.twitter.com%2Flongbeachgriffy&v=cuL2SZmLrR0&event=video_description&redir_token=EvkYDHO1dvjA3tsBZJ0_QUymTDN8MTUzNTU2MzkzMkAxNTM1NDc3NTMy" data-target-new-window="True" target="_blank" rel=" noopener" abs:href="https://www.twitter.com/longbeachgriffy">https://www.twitter.com/longbeachgriffy</a>

It seems like you added a new attribute called abs:href instead of editing href? Manually changing it while debugging makes it work, so the problem is that href exists.

https://github.com/Somethingweirdhere/NewPipe/tree/dirtyLinkFix

Check this out for a dirty fix. It's only here to demonstrate the problem, I feel like it's not regarded as best practice,

theScrabi · 2018-08-28T20:47:52Z

@Somethingweirdhere all right, I will fix it in the release_v0.14.0 branch.

theScrabi requested review from karyogamy and mauriciocolli August 23, 2018 12:41

karyogamy previously requested changes Aug 24, 2018

View reviewed changes

theScrabi mentioned this pull request Aug 24, 2018

Redesign #1461

Closed

1 task

TobiGr reviewed Aug 24, 2018

View reviewed changes

karyogamy reviewed Aug 25, 2018

View reviewed changes

theScrabi force-pushed the refactor_and_bugfix branch from 1d87f22 to f054999 Compare August 27, 2018 14:36

theScrabi added 10 commits August 27, 2018 16:37

fix exception when loading premium videos

2d029b9

fix cycling search results

b0efe49

rename Search Query handler

c2d11e7

fix layout width of currentPlayTime

0f517b8

fix linkhandling in description

badd4d3

bla

add reset extSD card folder dialog

44a71d8

add more debug statements to BasePlayer

98c65fb

fix exception on nothing found

eacbaa3

make dash parser ignore segmented streams

ce83fd9

fix thumbnail not shown in background player

2003f51

theScrabi force-pushed the refactor_and_bugfix branch from f054999 to 7b2661e Compare August 27, 2018 14:37

fixes acording to code review

b142936

fixes moreacording to code review fixed link handling once more

theScrabi force-pushed the refactor_and_bugfix branch from 7b2661e to b142936 Compare August 28, 2018 10:19

theScrabi removed the request for review from mauriciocolli August 28, 2018 10:19

theScrabi mentioned this pull request Aug 28, 2018

Release v0.14.0 #1635

Closed

theScrabi merged commit ad065e9 into dev Aug 28, 2018

theScrabi deleted the refactor_and_bugfix branch October 8, 2018 10:36

atpamat mentioned this pull request Nov 4, 2019

Limit amount of notification thumbnail updates in background player #2771

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and bugfix #1623

Refactor and bugfix #1623

theScrabi commented Aug 23, 2018

karyogamy left a comment

karyogamy Aug 24, 2018

theScrabi Aug 24, 2018

theScrabi Aug 24, 2018

karyogamy Aug 24, 2018

karyogamy Aug 24, 2018

theScrabi Aug 24, 2018 •

edited

Loading

karyogamy Aug 25, 2018

theScrabi Aug 26, 2018 •

edited

Loading

karyogamy Aug 24, 2018

theScrabi Aug 24, 2018

TobiGr Aug 24, 2018

theScrabi Aug 24, 2018

gregordr commented Aug 24, 2018 •

edited

Loading

theScrabi commented Aug 24, 2018

gregordr commented Aug 24, 2018

karyogamy Aug 25, 2018

karyogamy Aug 25, 2018

theScrabi commented Aug 27, 2018

gregordr commented Aug 27, 2018

theScrabi commented Aug 28, 2018

theScrabi commented Aug 28, 2018

gregordr commented Aug 28, 2018 •

edited

Loading

theScrabi commented Aug 28, 2018 •

edited

Loading

gregordr commented Aug 28, 2018

theScrabi commented Aug 28, 2018

Refactor and bugfix #1623

Refactor and bugfix #1623

Conversation

theScrabi commented Aug 23, 2018

karyogamy 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theScrabi Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theScrabi Aug 26, 2018 • 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

gregordr commented Aug 24, 2018 • edited Loading

theScrabi commented Aug 24, 2018

gregordr commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theScrabi commented Aug 27, 2018

gregordr commented Aug 27, 2018

theScrabi commented Aug 28, 2018

theScrabi commented Aug 28, 2018

gregordr commented Aug 28, 2018 • edited Loading

theScrabi commented Aug 28, 2018 • edited Loading

gregordr commented Aug 28, 2018

theScrabi commented Aug 28, 2018

theScrabi Aug 24, 2018 •

edited

Loading

theScrabi Aug 26, 2018 •

edited

Loading

gregordr commented Aug 24, 2018 •

edited

Loading

gregordr commented Aug 28, 2018 •

edited

Loading

theScrabi commented Aug 28, 2018 •

edited

Loading