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

Save NlohmannParser (JSON) settings #971

Merged
merged 4 commits into from
Apr 29, 2024

Conversation

MichelJansson
Copy link
Contributor

With this change, the NlohmannParser (json/bson/cbor/msgpack parsers) options are saved to the user settings file, making especially data streaming easier where we no longer have to specify this option after each restart of the app.

This PR partially solves #839: Settings are now saved, but not as part of the layout file.

Changes:

  • Settings saved when parser is instantiated.
  • Settings loaded when the options widget is accessed.

Design reasoning:

I debated over extending the ParserFactoryPlugin interface with void saveSettings() and void loadSettings() to make this feature explicit and controllable by the caller, that also would improve the interface for future plugins.
In the end I opted for this simpler more contained solution not to change the architecture, but with the downside of settings being loaded multiple times and both load and save being a side-effect.

I would be happy update the PR with the interface route if you think that is a better approach.

Thanks

- Save settings when parser is instantiated.
- Load settings when the options widget is accessed.
MichelJansson and others added 3 commits April 25, 2024 12:38
Now settings are saved separately for each encoding.

Co-authored-by: Davide Faconti <davide.faconti@gmail.com>
Encoding needs to be part of the base class to allow it to be
used in the constructor itself.
@facontidavide facontidavide merged commit e3ec836 into facontidavide:main Apr 29, 2024
8 of 13 checks passed
@MichelJansson MichelJansson deleted the save-json-options branch May 1, 2024 21:13
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