-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Refactor and bugfix #1623
Refactor and bugfix #1623
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -309,7 +309,6 @@ private void updateNotificationThumbnail() { | |||
@Override | |||
public void onLoadingComplete(String imageUri, View view, Bitmap loadedImage) { | |||
super.onLoadingComplete(imageUri, view, loadedImage); | |||
resetNotification(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I added the resetNotificateions() again, i found that the issues was that we didnt call updateNotificatinThumbnail() inside onUpdateProgress().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
final List<Throwable> exceptions = result.getErrors(); | ||
if (!exceptions.isEmpty() | ||
&& !(exceptions.size() == 1 | ||
&& exceptions.get(0) instanceof SearchExtractor.NothingFoundException)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should later consider removing this exception from the extractor. Is it still needed for some reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -1,8 +1,10 @@ | |||
package org.schabi.newpipe.player; | |||
|
|||
import android.content.ComponentName; | |||
import android.content.Context; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
app/src/main/res/values/strings.xml
Outdated
@@ -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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be Should the download place be reset? as reset behaves like set and both are irregular.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it.
I have taken a look at the link part.
|
True, but can the description view handle html? |
I have just taken a look at my old PR, and yeah it is possible: https://streamable.com/xo0q7 |
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to extract new File(folder, "NewPipe").getAbsolutePath()
to a variable since it is reused a couple lines above.
1d87f22
to
f054999
Compare
f054999
to
7b2661e
Compare
@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 :) |
@theScrabi Sure, here you go: NewPipe/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java Line 488 in 3a44e92
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.
This is how they look with the extractor right now. You have to make them look like this:
It's what I did here, in my old PR. It works, but I agree that the solution maybe could be prettier. |
@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
7b2661e
to
b142936
Compare
@karyogamy The changes you requested should be good by now, could you please release the block. |
@theScrabi Do you think you could Upload what you tried (NewPipe and extractor) so I can take a look at it? |
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.
@theScrabi I tried NewPipe Dev and NewPipe Extractor 3aa7ff6. When debugging, it looks like this:
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, |
@Somethingweirdhere all right, I will fix it in the release_v0.14.0 branch. |
This PR covers: