Sys-tray GUI side still pending

Note: although settings have a hierarchical appearance (i.e. their keys are structured with dots like client.primary_name), they are currently saved to disk in INI format (QSettings default). QSettings does not offer a YAML backend, but we should be able to register one.

This is ready to merge, as per discussion in IRC:

1. the petenv does not intersect with the systray yet, so the file-watcher bit is postponed until then (when the systray gets top-level commands).
2. I am creating a separate issue for saving settings in YAML format

This is ready to merge

Travis disagrees, looking into it.

It should be good now...

Codecov Report

Merging #805 into master will increase coverage by 0.06%.
The diff coverage is 75.23%.

@@            Coverage Diff             @@
##           master     #805      +/-   ##
==========================================
+ Coverage   68.59%   68.66%   +0.06%     
==========================================
  Files         179      188       +9     
  Lines        6289     6385      +96     
==========================================
+ Hits         4314     4384      +70     
- Misses       1975     2001      +26

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b04a7f5...e0504b5. Read the comment docs.

Codecov Report

Merging #805 into master will decrease coverage by 0.24%.
The diff coverage is 71.05%.

@@            Coverage Diff            @@
##           master    #805      +/-   ##
=========================================
- Coverage   68.94%   68.7%   -0.25%     
=========================================
  Files         182     190       +8     
  Lines        6401    6508     +107     
=========================================
+ Hits         4413    4471      +58     
- Misses       1988    2037      +49

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69cfb24...a16be82. Read the comment docs.

oO really ? let me check

@townsend2010 thanks for catching that, now I understand why I kept downloading images when switching branches :) (thought they were updating a lot)

I have pushed the fix. The issue was that, by setting organization name and domain in QCoreApplication, as advised in QSettings docs, I was also affecting other QStandardPaths. But simply leaving them out would result in an "Unknown Organization" folder for the settings.

Although the organization name can be passed directly to QSettings' ctor (rather than determined centrally) on mac the domain would be used instead, and QSettings does not provide a ctor for that. I ended up deriving a custom filename altogether, based on QStandardPaths::AppConfigLocation, the application name, and the suffix .conf (which QSettings used by default).

Note that this code will still have to change with #776 (the client will need access to both its and the daemon's settings file) and possibly for yaml backend.

@ricab,

Cool, thanks. There is also a merge conflict happening now too.

@townsend2010 rebased and force-pushed

@gerboland fair questions, I started typing answers a couple of times but I am not inspired ATM, I'll have to reply later.

Meanwhile I pushed fixes for compilation in buggy compilers ;)

@gerboland sorry for the long reply. I hope it is clear, but let me know if you would like to chat about this or if there are particular places in the code that I could explain better.

What brought me to this approach was really the combination of:

• having something globally accessible (to avoid param injection)
• being able to mock it

As you suggest, let's do away with option 1 (direct QSettings). It would not be mockable and it would also mean implementing extra logic (for checking, separate files (coming for the daemon), custom format (need to move to YAML)) in calling sites.

So we needed a wrapper. Your option 2 (regular interface + inheriting implementation and mock classes), was my departure point. It would normally mean passing SettingsInterface objects as a param whose ultimate type was determined elsewhere. As the config grew, multiple entities would need to access it, so the param could start polluting many signatures. Repeating the dependency would add entropy, but not much information, because we could approximate it by just saying "everyone depends on it" in a single place. I tried to explain this in the doc I wrote.

OK, so what if we just dropped the param but created a new object whenever needed? Well, we could not use the ctor directly if we wanted to mock it, because which type would we use? We would need some sort of factory. This factory would be an interface with an implementation for tests and one for real code. But which one do we call? Well, we'd need to pass one factory implementation in normal execution and another in tests. But for that we'd need to inject a dependency everywhere again. Basically we'd be back to the same problem as before.

What we needed was a way to say "here is the settings object, everyone can use it", in a single place. We'd determine the exact object (and thus its type) only once.

The Settings singleton does that. By default it provides a "real" Settings object: one of its own implementation, who does "the real job" we need. But it allows registering a derivation that overrides things for mocking.

Note that this is totally dependent on the dynamic polymorphism (the usual sort). The Settings class declares get/set as virtual and they are overridden by the mock. Relatively to what you propose, the only difference is that I skipped the pure interface part. That's perhaps not as clean looking, but it avoided another inheritance level and provided a ready default implementation that the mock could delegate to (which it does).

The rest is the implementation of the Singleton. That is where the static polymorphism comes into play, and only so we can have a Singleton base class that ensures it really has a single object. (Note: this restriction is actually a bit relaxed, to "only a single object at any point in time". There can be multiple objects over the course of a program, because I provide a protected reset, but that is really only there for mocking.)

To ensure that only one object is created, the Singleton needs to control ctor access. If its ctor was private, descendants could not be constructed (because they would not be able to call the parent's ctor). If it was protected, descendants would be able to construct as many as they liked, so they wouldn't be singletons.

So we use a different mechanism to restrict ctor calling that does not rely on visibilities at all. We actually declare it public, but give it a token parameter that callers need to be able to produce (here). Descendants then require this same token in their ctor and pass it to the parent ctor. We keep a single such token private in the Singleton and we provide it only in calls made by the Singleton itself. In other words, the constructors of Singleton derivations are only called by the Singleton (and intermediate des/ascendants).

Since the Singleton is the one creating the object, it needs to know its derived type. It gets that in the template parameter. But because we also allow mocking, we allow an additional template for the mock. That is the bit that introduces more template complexity.

The final bit is the PrivatePassProvider. This is the root of the singleton inheritance chain. It is just the class that provides the private token discussed above. We could pretty much paste its contents in the singleton directly, but that would make it even harder to read. Besides, having a separate class honors separation of concerns and modularity, so the PrivatePassProvider can be used for other purposes – I actually had it in my code for totally different purposes; it can be useful in general whenever we want fine control of accessibility that goes beyond what normal private/protected/public visibilities provide.

I suspect part of the "boiler-plate" you mention is due to separation of declaration from definition (even if the implementation is in the header). This is arguably less important today, with =default, constexpr, and default initializers, going the other way. But I think it still makes for a much clearer view. You look at the class and get an immediate overview of the interface without having to care about implementation details. If you want those, they are below, (but they do have to repeat templates and namespaces).

As for testability, it can be tested by attempting to compile something and see if it breaks. I provided examples in test_singleton.cpp and test_private_pass_provider.cpp. It is automated testing that is more difficult. I did that here with ifdefs and cmake. It seemed like too much here, but I can do it if requested. There is also DejaGNU but I am not familiar with it.

An alternative I thought of was a free factory function that would be "premocked" or have a header injected. Both sounded hackish and I was unsure if they were doable (I am not familiar with premock).

Another alternative that occurs to me now would be to provide copies or a static "template" object, rather than a reference to it as in a singleton. We would have to return unique_ptrs or something, to avoid chopping. I did not think of that thought. Nevertheless most of the complexity (for registration/resetting/mocking) would still be there. And having a single object makes it easier to reason about, either if it is a simple proxy to what could otherwise be global functions or if it gets state in the future.

Note to self: merge master and check if/where the new QString/fmt util can be used. TODO @ricab

Build succeeded

• codecov/project
• continuous-integration/travis-ci/push