transmission
Daemon default settings.json is incomplete
What is the issue?
On the initial transmission-daemon startup with a missing settings.json configuration file, the daemon generates one.
However, that file does not contain all the existing directives, but merely a subset of the possibilities, potentially misleading the user into thinking there is no other existing directive.
I would like to tackle this issue, but got lost in the way of understanding how the list of quarks end up in the file through macros and eventually tr_sessionGetDefaultSettings & tr_sessionSaveSettings.
If a good soul could provide me with pointers, I would be thankful!
Which application of Transmission?
transmission-daemon
Which version of Transmission?
4.0.0
One of the macOS volunteers will need to speak up about how this happens in that client :eyes:
On the initial transmission-daemon startup with a missing settings.json configuration file, the daemon generates one. However, that file does not contain all the existing directives, but merely a subset of the possibilities, potentially misleading the user into thinking there is no other existing directive.
Could you give a few examples of which entries are missing from the default settings.json?
Sure!
Here is a list of (seemingly not legacy, per docs) directives appearing in an old configuration file but not in the one generated by a fresh 4.0.0 instance:
-
alt-speed-enabled -
lazy-bitfield-enabled -
pidfile -
watch-dir -
watch-dir-enabled
I have no idea if I added those directives manually or if there were originally generated in the default configuration file of the old instances, as they are waaaaay too long running for my memory to cope.
I wonder about the course to be taken for that matter. Here are the options I explored:
1. Adding handling of optional (run-time) configuration directives (settings) during save
The problem is that some directives have no default, because they are not baked in the default hardcoded ones, but merely result from optional run-time flags, at least in case of the daemon, which I am using.
The obvious way would be to make those settings permanent by providing defaults in case optional flags are not being used.
2. Revamping configuration directive in-memory structures
To provide the first idea, I had to deep-dive into a lot of custom code around storing, loading and walking though configuration directives. I even made tools to display the contents of a settings dictionary and even printed steps of JSON deserialisation.
In short and namely, I had my share of tr_variant and jsonsl.
All this made me realise that, while this flexible, yet convoluted data structure had to be invented at the time of C, now that the project has switched over to >= C++17, existing advanced standard structures exist to handle maps or lists of variating types. On top of that, modern JSON loaders (de)serialisers exist.
To sum it all, modern libraries exist in order to alleviate one's load by avoiding the burden associated with hand-made structures/libraries maintenance.
Of course, I am not coming empty-handed with ideas only: we all know where that attitude usually lead. I tested the feasibility of such ideas, and I have hit quite a few road blocks, most of which (not all) I overcame. Modulo a few tweaks, it seems I could make a new settings system live along the current implementation and progressively replace it, step by step.
The question is: Shall I? In the end, nothing is worth being made if it does not end up integrated in the project.
This is drifting from the original subject, but I saw it as a perfect opportunity, an occasion to seize to address a tiny part of hand-made code calling to be standardised, mainly because it seems difficult to maintain/evolve.
- Do you concur on the rationale?
- If I were to submit a new settings management system proposal draft, would you be open to the idea of working towards it being eventually integrated?
@Berbe I'm open to the idea but would need more details. What do you have in mind?
I pushed a compiling/running PoC on my fork's settings/PoC branch which demonstrates my new proposed system working on the side of the old ones.
You will notice a couple overloads attempting to showcase the transition for session settings, and a call from the daemon component instantiating the new construct.
Again, this is a PoC made by a non-developer, hence please be kind/gentle, even if all this looks horrific! (o:
Having a (extensible) class would allow overloading from different components adding new settings (for instance the alt-speed ones), and the Settings class would make sure all the settings are then handled the same, for instance when saving to disk, resolving the original issue, source of this ticket.
If you like what you see, I trust the best way to iterate over it would be to split this issue into a refactoring one, merely referencing this issue as the "driving force"/parent behind it. If you don't... Welp, I tried, and we'll wait on others to suggest ideas/PRs.
@Berbe I'm a little unclear on what this branch is trying to solve?
No shade intended; I agree that the code shipping in Transmission right now could be improved on.
But could you give me a concise elevator pitch on the goal of this branch?
The goal it to transition from tr_based handcrafted structures/helpers functions to standard C++ ones for storing settings.
This would clarify/simplify how to deal with them, especially for cases in which some are added at runtime (like the alt-speed and daemon-specific ones, for instances).
Those daemon-added settings are, for example, not taken into account at save time, hence not appearing in the settings files. Rather than putting some effort in this machinery, I thought a sane first stamp would be to revamp their storage system.
My (very early) PoC is living on the side of the old one, so one could easily switch from one to the other for tests purposes and/or set regression tests up. I'm merely demonstrating feasibility than showing features, there.
Wow, when I saw #6449, I thought how did no one notice this earlier, but here we are
If these options are not generated when starting a fresh Transmission instance, then it probably means they are not defined at all in the X macros, so they are not recognised by Transmission at all.
I'll take a look.
@Berbe I've created a #6499 to address your original issue about missing options in the generated settings.json, and it will close this issue once merged.
If you are still interested in revamping the configuration directive in-memory structures, you are welcomed to to submit PRs or open issues!