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

Fix Settings import #7169

Merged
merged 2 commits into from
Sep 27, 2021
Merged

Fix Settings import #7169

merged 2 commits into from
Sep 27, 2021

Conversation

laksh-21
Copy link
Contributor

@laksh-21 laksh-21 commented Sep 25, 2021

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

  • The "Search Suggestions" setting in Settings > Content > Search Suggestions is a MultiSelectListPreference and that exports as a HashSet of all the enabled options.
  • The export was going through properly, but the import function did not have a case to handle the value if it was a HashSet in the key-value pair.
  • Since the incoming value was not being processed, the setting was just reverting to its default values.
  • I have added the case to handle when the value is a HashSet.
  • The Suppress annotation was added because value is of type any? and it needs to be converted to MutableSet<String>? which throws an Unchecked Cast warning. As far as I know, this warning can be suppressed without any problems, because the HashSet will always be convertible to MutableSet<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

@opusforlife2
Copy link
Collaborator

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.

@laksh-21
Copy link
Contributor Author

@opusforlife2 oh thanks! I'll keep that in mind next time. Didn't realize I made so many types T_T.
By the way, does the review usually take a long time?

@opusforlife2
Copy link
Collaborator

@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.

Copy link
Member

@litetex litetex left a 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 ✔️

@laksh-21
Copy link
Contributor Author

Hi @litetex I've made the requested changes :)

Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

LGTM

@Redirion Redirion merged commit 93aed9f into TeamNewPipe:dev Sep 27, 2021
This was referenced Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Search Suggestions" setting not imported
4 participants