[PRE-WORK] VPN-5854 - Refactor storage management #8820
Conversation
brizental
force-pushed
the
pre-experiments-work
branch
from
f090870 to
257ac57
Compare
Yeah, still learning about managing lifetimes without having my hand held by the compiler. (I miss Rust). Turns out I would have to cache the value of the setting in the SettigsGroup object for the previous design to work... I think that would not make much sense so I decided to diverge from the initial design and just expose the getter function itself.
brizental
force-pushed
the
pre-experiments-work
branch
from
7671674 to
6455399
Compare
|
|
||
| #include "helper.h" | ||
|
|
||
| class TestSettingsManager final : public TestHelper { |
There was a problem hiding this comment.
Great to see all these tests!
| Q_ASSERT(SettingsManager::instance()->getSetting("donotreset")); | ||
|
|
||
| // But it will clear the storage, if the setting is setup to do that. | ||
| QVERIFY(doReset->get().isNull()); |
There was a problem hiding this comment.
Since reset() did not unregister doReset, is the caller expected to call SettingsManager::createOrGetSetting again to re-register?
Should SettingsManager.sync() be expected to register this setting again (although the current implementation doesn't support that)?
There was a problem hiding this comment.
Nope, the caller is not expected to do so. I am thinking now that hardReset should not unregister the settings either. Neither method is guaranteed to be called in a situation where unregistering is the right move, which could lead to segfaults due to the unexpected behaviour. Remove also shouldn't unregister and so on.
Think about the situation where the user does a hardReset and then keeps using the app. (That is not possible currently, there is only hardResetAndQuit, but there is also nothing stopping us from implementing it.) In this situation suddenly calling any method on the SettingsHolder will result in a segfault.
Another option is to use shared pointers, but that would create the issue that a pointer to a setting exists that the SettingsManager is not aware of -- which invites another host of issues.
I'll go ahead and remove unregistering logic from everywhere but the destructor. Let me know if you disagree.
| * The only public constructor for Setting objects is | ||
| * SettingsManager::instance()->createOrGetSetting, therefore everytime a |
There was a problem hiding this comment.
The Settings constructor is public, so it could be created using new Settings(...). However, not a big deal.
There was a problem hiding this comment.
Yes. I think though, since the only public constructor requires SettingsConnector it might as well be private, because it's just too hard to use outside of the SettingsManager :D
This PR is on top of #8819, please ignore the first two commits on it.That has been merged.This PR is also pre-work for VPN-5854, in PR #8821.
Experiment feature have attached settings, but the way our SettingsHolder is implemented makes it hard to dynamically add settings. There is an underlying assumption that all settings will be defined at build time in the settingslist.h file.
In order to introduce the experimental settings features I had a few options that were all bit a hacky. Such as doing what we did for addon features and adding addon-specific (or in this case, experiment specific) functions to the SettingsHolder. Or just bypassing the SettingsHolder completely and just creating a different QSettings location for experiments settings.
None of these options made sense to me and I decided to refactor the whole settings managent in the app, to be more flexible towards dynamic creation of metrics.
The new design has the following pieces.
I have intentionally not renamed or moved the SettingsHolder class and files. The objective was to refactor everything without having to touch the whol codebase to change paths and calls to SettingsHolder functions. If we feel that is something we want to do, we can file a follow up ticket to do so.