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

Complex Settings #3025

Merged
merged 28 commits into from
Dec 1, 2024
Merged

Complex Settings #3025

merged 28 commits into from
Dec 1, 2024

Conversation

mrixner
Copy link
Contributor

@mrixner mrixner commented Nov 28, 2024

  • I have read the contributing guidelines, and I agree with the Code of Conduct.
  • Have you checked that there aren't other open pull requests for the same changes?
  • Have you tested that the committed code can be executed without errors?
  • This PR is not composed of garbage changes used to farm GitHub activity to enter potential Crypto AirDrops.
    Any user suspected of farming GitHub activity with crypto purposes will get banned. Submitting broken code wastes the contributors' time, who have to spend their free time reviewing, fixing, and testing code that does not even compile breaks other features, or does not introduce any useful changes. I appreciate your understanding.

This PR adds complex setting support to the settings engine (lists, dictionaries).

Things I'm planning to accomplish:

  • Add List Functions
  • Add List Tests
  • Add Dictionary Functions
  • Add Dictionary Tests
  • Make Sure Exporting These Settings Works Properly
  • Transfer Ignored Updates (Dictionary)
  • Code To Automatically Port Old Ignored Updates

I know we discussed using ISerializable but despite extensive searching I was unable to find a way to easily, automatically serialize ISerializable to JSON because apparently C#'s JSON thing doesn't support ISerializable. Therefore, I'm just converting everything to objects, casting them around and letting the JSON library serialize how it wants.

@marticliment
Copy link
Owner

Yes, this looks good for me.
Perhaps you could (in order to not have a huge file with the entire settings code) mark the settings class as partial, and have List-related and Dictionary-related settings be implemented in their own files. You can find an example on what I mean on src/UniGetUI/Pages/DialogPages/DuialogHelper_*.cs, which are different files that define different parts of the same static class

@mrixner
Copy link
Contributor Author

mrixner commented Nov 29, 2024

Do you want the installation options to continue to not be backed up in the settings? Because if so I should update IgnoredSettings (SettingsPage.xaml.cs:537) to include the installation options. I think probably, since if settings are used to transfer UniGetUI configurations across computers, installation options would be different across computers.

@mrixner
Copy link
Contributor Author

mrixner commented Nov 29, 2024

Actually, unless you would like me to port them, I think I'll just leave the installation options where they are because the current storage method because it's pretty ingrained. But I'd be happy to port them if you want consistency.

@marticliment
Copy link
Owner

Actually, unless you would like me to port them, I think I'll just leave the installation options where they are because the current storage method because it's pretty ingrained. But I'd be happy to port them if you want consistency.

I'd leave them as they are, as the method use is very customized for the use cases of the settings.

@marticliment
Copy link
Owner

Do you want the installation options to continue to not be backed up in the settings? Because if so I should update IgnoredSettings (SettingsPage.xaml.cs:537) to include the installation options. I think probably, since if settings are used to transfer UniGetUI configurations across computers, installation options would be different across computers.

I reckon it makes sense that installation options are exported via package bundles, rather than on the settings

@marticliment
Copy link
Owner

I will merge both of the PRs before beta2, so there will not be any need to migrate any settings.
I reckon it would be a good idea to first merge this PR, so then you can pull main from the "remove desktop icons" PR, what do you think?

@mrixner
Copy link
Contributor Author

mrixner commented Nov 29, 2024

Yeah, sounds good! Thanks.

@mrixner mrixner marked this pull request as ready for review November 29, 2024 20:31
marticliment
marticliment previously approved these changes Dec 1, 2024
@marticliment marticliment merged commit 5ee697f into marticliment:main Dec 1, 2024
2 checks passed
@mrixner
Copy link
Contributor Author

mrixner commented Dec 1, 2024

Thanks!
Note: you added a ClearList call in the tests, but I explicitly didn't have one there so it would work as a test of making sure an empty list is returned when a setting doesn't exist.
image

@marticliment
Copy link
Owner

I did, because if the tests failed and test files remained the tests would fail infinitely

@mrixner
Copy link
Contributor Author

mrixner commented Dec 1, 2024

OK, that's what I figured, just confirming.

@mrixner mrixner deleted the complex-settings branch December 1, 2024 22:36
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