-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add download option to long-press menu #6563
Add download option to long-press menu #6563
Conversation
cc6f35f
to
4a0c903
Compare
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.
Thanks for the PR! Please also use the new method in VideoDetailFragment
so as to reduce code duplication.
app/src/main/java/org/schabi/newpipe/util/NavigationHelper.java
Outdated
Show resolved
Hide resolved
|
||
try { | ||
// Get the sortedVideoStreams and selectedVideoStreamIndex using ListHelper | ||
final ArrayList<VideoStream> sortedVideoStreams = new ArrayList<>( |
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.
The variable's type should be List<VideoStream>
, as we do not care about the implementation:
final ArrayList<VideoStream> sortedVideoStreams = new ArrayList<>( | |
final List<VideoStream> sortedVideoStreams = new ArrayList<>( |
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.
Made the changes that you suggested. However, using the same code in VideoDetailFragment
will break MainActivity
NewPipe/app/src/main/java/org/schabi/newpipe/MainActivity.java
Lines 619 to 621 in 484c852
if (fragment instanceof VideoDetailFragment) { | |
((VideoDetailFragment) fragment).openDownloadDialog(); | |
} |
Than you i have been waiting for this for a long time |
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.
lgtm, tested it, works. All good. Thank you
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.
Thank you! Have you tested that it works correctly in the feed fragment with fast mode enabled?
app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java
Outdated
Show resolved
Hide resolved
@@ -371,6 +371,7 @@ protected void showStreamDialog(final StreamInfoItem item) { | |||
)); | |||
} | |||
entries.add(StreamDialogEntry.open_in_browser); | |||
entries.add(StreamDialogEntry.download); |
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.
Have you checked if the download option can be added to other places where StreamDialogEntry
is used?
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.
On it. I'll check it out.
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.
Not sure about what I'm supposed to do here. @Stypox please help me understand what I should check
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 just made a similar change in #7160. You add your entry in BaseListFragment
, but there are a bunch of other locations that have similar code (adding options to the long-press menu for stream items).
Do a search for add(StreamDialogEntry.
and you should turn them all up.
You'll also want to test that it actually works in each other place, because some of those places may have different data available. In most cases besides BaseListFragment
, the name of the fragment is a good indicator of where to find it in the app.
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.
@gitryder Android Studio tells me that StreamDialogEntry
is used in these places:
@gitryder any update? |
Still working on it. Gimme only some time more |
@gitryder ok, no problem :-) |
This comment was marked as off-topic.
This comment was marked as off-topic.
I lost my local copy that had this branch on it. How do I push to this same PR now? |
Just clone your repo again and |
No progress since ~6months. |
How do I download it pls tell really need it :( |
@Hanankhan1983 this was added in #8397 and will be present in the next release; you can download an APK built by our CI from here: https://github.com/TeamNewPipe/NewPipe/suites/7347347309/artifacts/297870978 |
Thanks! |
What is it?
Description of the changes in your PR
Add option to download the video when long-pressing items.
Fixes the following issue(s)
APK testing
On the website the APK can be found by going to the "Checks" tab below the title and then on "artifacts" on the right.
Due diligence