-
-
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
Fix Settings import #7169
Fix Settings import #7169
Conversation
Hi, @laksh-21, and thank you for contributing to Newpipe! I think your PR description was well written. I made some small edits to make it look better, which you can see in detail using the edit history. |
@opusforlife2 oh thanks! I'll keep that in mind next time. Didn't realize I made so many types T_T. |
@laksh-21 That completely depends on developer availability. Since this is a completely voluntary project, that could mean hours/days/weeks or even months. But usually small PRs like yours get reviewed fairly quickly. |
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.
Code generally looks good to me (LGTM)
However I request some minor changes.
Tested this (with my minor changes) and works as expected ✔️
app/src/main/java/org/schabi/newpipe/settings/ContentSettingsManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/settings/ContentSettingsManager.kt
Outdated
Show resolved
Hide resolved
Hi @litetex I've made the requested changes :) |
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
What is it?
Description of the changes in your PR
MultiSelectListPreference
and that exports as aHashSet
of all the enabled options.HashSet
in the key-value pair.HashSet
.Suppress
annotation was added because value is of typeany?
and it needs to be converted toMutableSet<String>?
which throws anUnchecked Cast
warning. As far as I know, this warning can be suppressed without any problems, because theHashSet
will always be convertible toMutableSet<String>
.Fixes the following issue(s)
APK testing
APK for the app: debug_app.apk
Edit: added the APK
Also, this is my first time contributing :). Can I get a feedback on how well this PR description was written?
Due diligence