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

Option to reset settings #9236

Merged
merged 18 commits into from
Mar 28, 2024
Merged

Conversation

vincetzr
Copy link
Contributor

@vincetzr vincetzr commented Oct 28, 2022

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

  • Add a new setting: reset settings. This will delete the shared preference and restarts the app. An alert dialogue will appear to prevent accidental click.
  • Created new settings category: Backup and restore
    Following settings have been move to the new category:
    • import database (from ContenttSettings)
    • export database (from ContenttSettings)
    • reset settings (new)

Before/After Screenshots/Screen Record

Before After After video
Before After
Screen.Recording.2022-10-29.at.10.04.33.am.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

Copy link
Member

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Thank you for the PR.

Please remember to make new strings translatable.

There are no debug settings in the release APKs. Therefore, placing this action into the debug settings does not solve the problem or use case mentioned in the issue. We should find a better place for it. Unfortunately, I cannot think of a category that somehow works on this. IMO it's time to create a new settings category (Backup & Restore maybe?). It should contain the backup actions which are currently placed in history & cache.

@ All, what do you think on that suggestion?

<Preference
android:key="@string/reset_settings"
android:title="@string/settings_category_reset_title"
app:iconSpaceReserved="false" />
Copy link
Member

Choose a reason for hiding this comment

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

We need to ensure that the title is fully displayed on small devices:

Suggested change
app:iconSpaceReserved="false" />
app:singleLineTitle="false"
app:iconSpaceReserved="false" />

Copy link
Contributor Author

@vincetzr vincetzr Oct 29, 2022

Choose a reason for hiding this comment

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

I see. Thank you for pointing it out. I have committed the changes.

Copy link
Contributor

@plasticanu plasticanu Oct 29, 2022

Choose a reason for hiding this comment

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

I see. Thank you for pointing it out. I have committed the changes.

I think you can start working on the solution TobiGr mentioned above. I will help you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the deadline is passed and we have more assignments to do, also 4 final exams and a 2000-word essay are coming. We have to wait until 15 Nov or later to continue to work on this PR

Copy link
Member

@Stypox Stypox Oct 30, 2022

Choose a reason for hiding this comment

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

Ok, don't worry, and good luck with exams! ;-)

I will turn this PR into a draft for now, feel free to write a comment once you get back on it, so that we see it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. Thank you.

@TobiGr TobiGr added the feature request Issue is related to a feature in the app label Oct 29, 2022
@sonarcloud
Copy link

sonarcloud bot commented Oct 29, 2022

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 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Stypox Stypox marked this pull request as draft October 30, 2022 21:25
@ghost
Copy link

ghost commented Feb 28, 2023

You can already reset your settings by clearing your storage for the app.

EDIT:I forgot to mention it would also clear out all your history for the app! Sorry for the confusion 😞

@TobiGr TobiGr marked this pull request as ready for review September 17, 2023 15:22
@TobiGr
Copy link
Member

TobiGr commented Sep 17, 2023

  • Rebased
  • Created new settings category: Backup and restore
    Following settings have been moved to the new category:
    • import database (from ContenttSettings)
    • export database (from ContenttSettings)
    • reset settings (from DebugSettings)

Copy link
Member

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

LGTM
Please wait for approval by second maintainer regarding new settings structure

@opusforlife2
Copy link
Collaborator

I would say let's have an extra precautionary step before the task is executed. The alert dialog should have a check box that needs to be checked before the Yes button can be tapped, until which it will remain greyed out.

@TobiGr
Copy link
Member

TobiGr commented Sep 18, 2023

Having one dialog with CANCEL and OK is the same flow used for clearing the watch and search history as well as the playback positions. I think it should be consistent with these similar actions.

@opusforlife2
Copy link
Collaborator

Well, there should be a check box for those too. Any destructive action should have this double check. It's different from PC where you usually have to move the pointer quite a distance to execute the task. On Android, mis-taps are far more likely to occur. (e.g. just today I accidentally deleted an important SMS from its heads-up notification and had to request a re-send.)

@sonarcloud
Copy link

sonarcloud bot commented Sep 21, 2023

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 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@github-actions github-actions bot added the size/large PRs with less than 750 changed lines label Mar 28, 2024
Copy link

sonarcloud bot commented Mar 28, 2024

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.

Thank you!

@Stypox Stypox merged commit 2af95cc into TeamNewPipe:dev Mar 28, 2024
5 of 6 checks passed
@Stypox Stypox mentioned this pull request Apr 1, 2024
8 tasks
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 size/large PRs with less than 750 changed lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to reset settings.
5 participants