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

Load uploaderUrl when showing Channel Details from Play Queue #7538

Merged

Conversation

ktprograms
Copy link
Contributor

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 #6919.

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

  • Move the code that conditionally fetches the uploaderUrl and saves it to the database into a helper class + method.
  • Use the saveUploaderUrlIfNeeded method in QueueItemMenuUtil when selecting Show Channel Details in order to load the uploaderUrl if it isn't available in the database.

Before/After Screenshots/Screen Record

Android Emulator screen recording seems to be broken, I'll do my best to describe the before and after in text.

  • Before: When selecting Show Channel Details on a video that doesn't have uploaderUrl in the database, it will go to the channel fragment, but the uploaderUrl will be null, so it will cause a network error.
  • After: When selecting Show Channel Details on a video that doesn't have uploaderUrl in the database, it will fetch the uploaderUrl and display a loading toast, then open the channel fragment.

Fixes the following issue(s)

Did not find any issues that will be fixed by this PR.

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

@XiangRongLin XiangRongLin added the bug Issue is related to a bug label Dec 14, 2021
@litetex litetex marked this pull request as draft December 14, 2021 18:49
@litetex
Copy link
Member

litetex commented Dec 14, 2021

Detected a TODO: https://github.com/TeamNewPipe/NewPipe/pull/7538/files#diff-212f79b4111281cdfbffed5f79bca8922f99be14642ef90d524e6c9de9a436f7R61

Assuming this PR is not yet in a reviewable state and converted it to a draft.

@ktprograms
Copy link
Contributor Author

@litetex This TODO was from the previous PR (#6919), and it isn't critical to operation of this functionality.

I can't figure out how to open the Error Activity, can you either give an example code or link to some code that does this?

@litetex
Copy link
Member

litetex commented Dec 16, 2021

@ktprograms

I can't figure out how to open the Error Activity, can you either give an example code or link to some code that does this?

Basically take a look at the ErrorUtil class
Stypox refactored that recently, you may also checkout #7482; There are some usage examples 😄

@ktprograms
Copy link
Contributor Author

@litetex Unfortunately, I'm not sure there's a good way to show the error.

Using ErrorUtil opens the full blown error reporting activity, which isn't necessary since most of the time it would be a network error which can be retried.

I tried using ErrorPanelHelper, but that doesn't work because this part of the code doesn't have access to a Fragment.

I also can't use ErrorUtil.showSnackbar because it can't seem to find the correct rootView.

@litetex
Copy link
Member

litetex commented Dec 20, 2021

@Stypox

I also can't use ErrorUtil.showSnackbar because it can't seem to find the correct rootView.

Do you have an idea how to fix this?

@Stypox
Copy link
Member

Stypox commented Jan 3, 2022

@ktprograms I don't know if it would be possible to open a snackbar in that case. In order to open a snackbar you need a Context which is instanceof Activity, or a Fragment. Only in those cases (and if the activity/fragment has a valid rootView, which it usually has) can the snackbar be shown. But since you call showSnackbar with a "random" Context that might not be instanceof Activity, it won't work. Note that Fragment.getContext() does not necessarily return a Context which is instanceof Activity, only Fragment.getActivity() does, if != null. So if you might need to change the methods you created a bit, in order to make sure not to pass fragment.getContext() to showSnackbar but pass the original fragment instead. Also, you should ensure that the context in QueueItemMenuUtil is instanceof Activity; if is not always such, also change QueueItemMenuUtil.openPopupMenu accordingly.

If doing these things turns out to require changing too many lines and duplicate too much code, just use an error notification, that can be shown just by using a context.

@ktprograms
Copy link
Contributor Author

@Stypox I'll look more into this tomorrow, but what do you mean by just use an error notification? Do you mean just standard Android notifications or something else?

@ktprograms
Copy link
Contributor Author

Would it be better to merge this first as it fixes the bug that the channel details aren't properly loaded, and move the error handling to another PR? (BTW the TODO was from my first PR about Show Channel Details)

@Stypox
Copy link
Member

Stypox commented Jan 7, 2022

@ktprograms just replace ErrorUtil.showSnackbar(context, errorInfo) with ErrorUtil.createNotification(context, errorInfo)

@ktprograms ktprograms force-pushed the fix-queue-channel-details-not-in-db branch from 37762f5 to 835e2f2 Compare January 8, 2022 00:22
@ktprograms
Copy link
Contributor Author

replace ErrorUtil.showSnackbar(context, errorInfo) with ErrorUtil.createNotification(context, errorInfo)

Done. Ready for review, please.

@ktprograms ktprograms force-pushed the fix-queue-channel-details-not-in-db branch from 835e2f2 to c39c0b1 Compare January 8, 2022 00:49
@opusforlife2
Copy link
Collaborator

Done. Ready for review, please.

Then this doesn't need to be a draft anymore.

@opusforlife2 opusforlife2 marked this pull request as ready for review January 8, 2022 10:47
@opusforlife2 opusforlife2 requested a review from Stypox January 8, 2022 11:01
@ktprograms ktprograms force-pushed the fix-queue-channel-details-not-in-db branch from c39c0b1 to b0f2287 Compare January 10, 2022 00:30
@ktprograms ktprograms force-pushed the fix-queue-channel-details-not-in-db branch from b0f2287 to 607d995 Compare January 11, 2022 00:44
@ktprograms ktprograms requested a review from litetex January 11, 2022 00:45
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@ktprograms ktprograms force-pushed the fix-queue-channel-details-not-in-db branch from 607d995 to 973fd8f Compare January 25, 2022 02:49
@ktprograms ktprograms requested a review from Stypox January 25, 2022 02:51
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 ktprograms force-pushed the fix-queue-channel-details-not-in-db branch from 973fd8f to 1e652b1 Compare January 25, 2022 03:00
@ktprograms
Copy link
Contributor Author

How would I fix this warning: https://sonarcloud.io/project/issues?id=TeamNewPipe_NewPipe&issues=AX6PN5ztwvdSy9w-T0sr&open=AX6PN5ztwvdSy9w-T0sr&pullRequest=7538 ? It is in the else branch of isNullOrEmpty so it can't be null, but how do I indicate that to the analysis?

@XiangRongLin
Copy link
Collaborator

How would I fix this warning: https://sonarcloud.io/project/issues?id=TeamNewPipe_NewPipe&issues=AX6PN5ztwvdSy9w-T0sr&open=AX6PN5ztwvdSy9w-T0sr&pullRequest=7538 ? It is in the else branch of isNullOrEmpty so it can't be null, but how do I indicate that to the analysis?

That does not seem to be possible for you: https://community.sonarsource.com/t/sonarqube-does-not-detect-null-check-by-a-method-and-showing-possible-nullpointerexception/28809
I marked it as false positive for now

@ktprograms
Copy link
Contributor Author

ktprograms commented Jan 25, 2022 via email

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.

Code looks good, thank you! I tested and it works well (even though now the problem with QueueItemMenuUtil is already disappeared thanks to #7036 which makes sure items with full stream info to be enqueued).

@Stypox Stypox merged commit 17c0fff into TeamNewPipe:dev Jan 26, 2022
@ktprograms
Copy link
Contributor Author

ktprograms commented Jan 26, 2022 via email

@Stypox Stypox mentioned this pull request Jan 27, 2022
10 tasks
This was referenced Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants