-
-
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
Show "Hold to enqueue" tip in local playlists #9196
Show "Hold to enqueue" tip in local playlists #9196
Conversation
Does this respect the "show tip" toggle in Appearance settings? |
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 toast should only be shown if the user did not disable enqueue tips, as opusforlife2 pointed out. Refer to the show_hold_to_append_key
shared preferences key.
I've now made the toasts only appear when enqueue tips are enabled |
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, almost good
if (PreferenceManager.getDefaultSharedPreferences(activity) | ||
.getBoolean(getString(R.string.show_hold_to_append_key), true)) { | ||
Toast.makeText(activity, R.string.hold_to_append, Toast.LENGTH_SHORT).show(); | ||
} |
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.
Instead of duplicating these four lines of code three times, it's better if you create a function (e.g. right after the handleResult
) named showHoldToAppendTipIfNeeded
containing these four lines, and then you can just call it in the three places.
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 feedback, I've put those four lines of code in a function
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, looks good to me ;-)
I tested myself since the CI is not working.
Note that this change only applies to the local playlist fragment, while it should apply to other list fragments, too (e.g. history, remote playlist, channel, ...). If you want to fix those fragment, too, go ahead.
@Jfax510 ANU student? |
Yes, I did this issue as part of a group assignment |
What is it?
Description of the changes in your PR
Before/After Screenshots/Screen Record
BeforeTest.mov
AfterTest.mov
Fixes the following issue(s)
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