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

Added Reset Buttons for every Settings Screen #992

Open
wants to merge 8 commits into
base: redesign
Choose a base branch
from

Conversation

flloschy
Copy link

Fyi: Some already existing update functions I used dont update the UI, dont know how to fix that

Also idk why pubspec.lock got updated, i guess thats normal?

Please double check the defaults ive put there, I believe they are the real defaults since I copied the settings I had which I didnt change much if at all

@Chaphasilor
Copy link
Collaborator

Thanks for the PR!

Changes look good, here are a few comments:

  • The pubspec.lock file contains the current versions of all used packages (dependencies). In the main package file, pubspec.yaml, we list each needed package along with a version selector, usually something like ^1.2.3, which means it can be updated to 1.x.y if a new version becomes available. This way packages can be automatically updated without explicitly changing the version. pubspec.lock then keeps track of which version was actually used. So yes, it's usually okay if it changes.
  • The settings that don't automatically update are stateful child widgets, so their state (e.g. current text in a TextField) is preserved even when the parent widget is rebuilt. You can force a rebuild of the children by explicitly giving them a key, and changing that key in the setState() method. Here's a longer explanation
  • You can run dart format . in your terminal to automatically apply the correct formatting to your code. Your editor (probably VS Code) also usually has an option to "format on save", which makes sure the code is correctly formatted right-away.
    If you run dart format ., make sure to do it at the very start or end, in a separate commit, so that it is easy to see what you actually changed aside from the formatting.
  • Right now we're just using same values as the current default and manually setting them to reset everything. If someone changes the default, the reset would still use the old value. I think we should refactor things so that we directly use the default values from finamp_models.dart to stay consistent. Right now those are private (leading underscore, so only available in that file), which we would need to change. If you want to give it a try, go ahead! Otherwise I can also try it. I suggest we create a new class that contains all these values as static const properties.

@Chaphasilor
Copy link
Collaborator

Oh and I just noticed you already added a button for resetting everything! That would've been my next step. Awesome!

@Chaphasilor
Copy link
Collaborator

Just tested it out, everything seems to work. I have one more suggestion:
A simple confirmation dialog would be nice, something like "Do you want to reset these settings back to defaults?" With "Reset" and "Cancel" options. To do this, the reset button could be made into a new widget (SettingsResetButton) that is passed a function, then shows the dialog on tap, and calls the function if the dialog is confirmed.
That would also get rid of a ton of duplicate code :)
There already is a ConfirmationPromptDialog that you can use for this!

@flloschy
Copy link
Author

flloschy commented Dec 30, 2024

Instead of a new Class I made a function in the settings helper, kinda against convention but i felt like its cleaner than having a new class.
At least as long as this button is only used for this specific use case.

Because the reset action is now in a generic function I didn't implement the UI reloading, cant think of a nice way of doing this now :/ I did find a way, just not as nice as i would like it to be

And of course app localization needs to be added for the 4 new strings that came with the function, dont know how though

@Chaphasilor
Copy link
Collaborator

Thanks! I mean it's not like we need to save on components, so maybe a regular widget is better :)

For the state reloading, you can simply pass an anonymous function to makeSettingsResetButtonWithDialog() (or the new widget), like so:

FinampSettingsHelper.makeSettingsResetButtonWithDialog(
  context,
  () {
      setState(() {
        FinampSettingsHelper.resetAlbumSettings();
        // optionally assign a new key for child widgets here
      });
    },
  )

That should work just fine.
Edit: I just saw that you did just that in the latest commit ^^

As for localizations, you go into app_en.arb and add the missing tokens either at the end or next to some similar ones. Always add the token itself and some metadata (prefixed with an @) that includes a description of how/where the token will be used.
Then you can either run the "Generate Localizations" command (in VS Code), or use the regular code generation command (from the contributing guidelines): dart run build_runner build --delete-conflicting-outputs. This will register the new tokens so that you can use them in the code. After changing or adding a token, you need to manually hot-reload, since changing the .arb file won't trigger a reload.

@flloschy
Copy link
Author

I believe thats everything done you asked for :D

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.

2 participants