Skip to content
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 channel details option to long-press menu #5851

Merged
merged 12 commits into from
Mar 28, 2021

Conversation

triallax
Copy link
Contributor

@triallax triallax commented Mar 18, 2021

What is it?

  • Feature (user facing)

Description of the changes in your PR

Add option to open channel details page 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

@Stypox
Copy link
Member

Stypox commented Mar 18, 2021

Maybe try with getParentFragmentManager()

@triallax
Copy link
Contributor Author

Unfortunately that doesn't fix the issue.

@Stypox
Copy link
Member

Stypox commented Mar 18, 2021

With fragment.getActivity().getSupportFragmentManager() it works (this is strange, though)

@triallax triallax marked this pull request as ready for review March 18, 2021 15:06
@triallax triallax changed the title Add channel channel details option to long-press menu Add channel details option to long-press menu Mar 19, 2021
@triallax triallax added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels Mar 19, 2021
@triallax triallax force-pushed the add-channel-to-longpress-menu branch from 15df526 to a7c68c1 Compare March 21, 2021 16:32
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@triallax

This comment has been minimized.

@triallax

This comment has been minimized.

@Stypox
Copy link
Member

Stypox commented Mar 25, 2021

@mhmdanas please fix the issues pointed out by Sonar

@triallax
Copy link
Contributor Author

triallax commented Mar 25, 2021

I fixed the braces "code smell," but kept the other two because one is a TODO and the other is because I'm following the naming convention of the rest of the file.

@Stypox
Copy link
Member

Stypox commented Mar 25, 2021

Do you think this can be merged even though there is an unsolved TODO?

@triallax
Copy link
Contributor Author

I searched quite a bit and failed to find any explanation. Can I just replace the TODO comment with something like "For some reason getParentFragmentManager() doesn't work, but this does"?

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I just replace the TODO comment with something like "For some reason getParentFragmentManager() doesn't work, but this does"?

@mhmdanas ok, that's fine

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :-)

@Stypox Stypox merged commit 6a9cae3 into TeamNewPipe:dev Mar 28, 2021
@triallax triallax deleted the add-channel-to-longpress-menu branch March 28, 2021 16:49
This was referenced Apr 11, 2021
@opusforlife2
Copy link
Collaborator

This option doesn't appear in the Feeds (What's New or any other). Known issue? Postponed?

@triallax
Copy link
Contributor Author

triallax commented Apr 12, 2021

@opusforlife2 not known, at least to me.

The only way that would happen is if calling getUploaderUrl on the feed items returns null, which may sound unlikely, but is the only possibility here. I'll investigate later.

Edit: what service did you reproduce that on? Or is it all of them?

@opusforlife2
Copy link
Collaborator

what service did you reproduce that on?

I tried it with a feed with only YouTube channels in it. I haven't tried it with any other service.

@triallax
Copy link
Contributor Author

The reason why this doesn't work with feeds is because the database doesn't store the feed items' uploader URLs. I don't think going back through all the feed items in feeds like "What's New" to add the uploader URL is a good idea. Maybe we could only add the URLs for new feed items? Anyway, I'll open an issue to continue the discussion there.

@opusforlife2
Copy link
Collaborator

@mhmdanas Please ping me in that issue so I can follow it when you open one.

@opusforlife2
Copy link
Collaborator

Maybe we could only add the URLs for new feed items?

Yeah, adding them when the user initiates a feed refresh seems like a practical solution.

@triallax
Copy link
Contributor Author

To be clear, I did not mean to add it for all previous feed items, but only for new ones (i.e. ones added in a feed refresh).

@opusforlife2
Copy link
Collaborator

I got what you meant. We're on the same frequency. :3

tossj pushed a commit to tossj/NewPipe-legacy that referenced this pull request Apr 23, 2021
Add dialog item to open channel details
Use `List` as type of `entries`
Put channel details item last
Only show channel option when channel is present
@Plugdemholes-Imeanpatchdembugs

@mhmdanas
Thanks for the implementation.
Usage has become far more frictionless than ever before since the implementation! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Longpress (menu) option for direct channel videos overview access (tablet optimisation)
5 participants