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 functionality for General Settings Page #1664

Merged
6 commits merged into from
Mar 25, 2020

Conversation

ghost
Copy link

@ghost ghost commented Mar 24, 2020

Summary of the Pull Request

  • Added functionality for General Settings Page

  • Settings Include: Run on startup, change the theme, links to issues and bug reports and restart as admin.

  • I would like feedback on the architecture, implementation, and design. I will enforce more good practices on the upcoming PRs.

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

  • To test the code, re-add refference to the IPCLib dll found in x64/ directory to the Runner.

Validation Steps Performed

  • Validdated output on the UI and the json settings file found in AppData/Local/Microsoft/PowerToys/settings.json

@ghost ghost requested review from crutkas, yuyoyuppe, enricogior and a team March 24, 2020 06:54
Copy link
Collaborator

@yuyoyuppe yuyoyuppe left a comment

Choose a reason for hiding this comment

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

LGTM. Out of curiosity, what are the main differences between System.Text.JSON and Json.NET libraries? I've noticed that we use the former one in FZE, while the latter in newer modules.

@crutkas
Copy link
Member

crutkas commented Mar 24, 2020

LGTM. Out of curiosity, what are the main differences between System.Text.JSON and Json.NET libraries? I've noticed that we use the former one in FZE, while the latter in newer modules.

IMO JSOn.net is far more the standard and is faster. Template studio uses this.

We should standardize across the code base IMO too

@ryanbodrug-microsoft
Copy link
Contributor

System.Text.JSON

LGTM. Out of curiosity, what are the main differences between System.Text.JSON and Json.NET libraries? I've noticed that we use the former one in FZE, while the latter in newer modules.

@laviusmotileng-ms : This is a good call out. If possible we should be using System.Text.JSON. It was built into .net core 3, partly to alleviate issues with dependent packages using conflicting versions of json.net (newtonsoft.json). System.Text.Json claims performance improvements over json.net. With that said, I do not personally have enough experience with System.Text.JSON to say that it is not without some 'gotcha's, but I would recommend it as the default json serializer for new projects.

@ryanbodrug-microsoft
Copy link
Contributor

LGTM. Out of curiosity, what are the main differences between System.Text.JSON and Json.NET libraries? I've noticed that we use the former one in FZE, while the latter in newer modules.

IMO JSOn.net is far more the standard and is faster. Template studio uses this.

We should standardize across the code base IMO too

@crutkas Are we sure that JSON.net is faster? I would actually lean towards System.Text.JSON. Especially if we are looking to standardize across the codebase as it grows. I've been bitten before where I've been unable to use certain packages because of specific version dependencies on json.net. Although, like I said in lavius's comment. I don't have much experience with System.Text.JSON and given that it's relatively new, there still may be dragons.

@crutkas
Copy link
Member

crutkas commented Mar 24, 2020

Let’s then do the system one. I have always had good experiences. I just think we should standardize across everything.

@ghost
Copy link
Author

ghost commented Mar 24, 2020

LGTM. Out of curiosity, what are the main differences between System.Text.JSON and Json.NET libraries? I've noticed that we use the former one in FZE, while the latter in newer modules.

@ryanbodrug-microsoft I tried using the .Net core 3 Json library with no success. The library currently does not support the parsing of dynamic objects. This is an open issue in the .net runtime project. To continue using this will mean that I will have to add class models for each settings file. I am adding that now and will push the changes.

@ryanbodrug-microsoft
Copy link
Contributor

LGTM. Out of curiosity, what are the main differences between System.Text.JSON and Json.NET libraries? I've noticed that we use the former one in FZE, while the latter in newer modules.

@ryanbodrug-microsoft I tried using the .Net core 3 Json library with no success. The library currently does not support the parsing of dynamic objects. This is an open issue in the .net runtime project. To continue using this will mean that I will have to add class models for each settings file. I am adding that now and will push the changes.

I think class models is probably the better way to go anyway. I'd prefer to get a compile error if a setting is accidentally removed instead of a runtime error using dynamic objects. Hopefully this isn't much work.

@ghost ghost requested a review from crutkas March 25, 2020 02:54
@ghost ghost merged commit a27e8bc into dev/settingsV2 Mar 25, 2020
@ghost ghost deleted the user/lamotile/general_settings branch March 25, 2020 02:55
ghost pushed a commit that referenced this pull request Apr 7, 2020
* archive

* formmated code

* reverted changes to test class file.

* reverted changes to test file: reverted name

* added class models and updated link

* removed test console project
This pull request was closed.
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.

3 participants