Set up a .settings file

[retailcoder]

ref. discussion on #3534

Let's put all the default settings in a .net-standard .settings file (i.e. accessible from project properties), at "application" level; that file would end up in the RD install folder and in no way would interfere with user settings. The contents of that file would be version-specific, and the file itself would be essentially treated as read-only: nothing ever writes to it.

This will require changes to all inspection classes (they all pass a hard-coded default severity through the base class constructor!), and remote a TON of shitty default-config setup code, notably for hotkeys.

Then the mechanics for loading user settings become brain-dead simple:

• Load all the defaults (inspection severities, todo markers, hotkeys, indenter settings, etc.) from the application settings
• See if there's a rubberduck.config user settings file (i.e. the xml file we're currently using); if so, load what can be loaded & override the loaded defaults. If user settings contain unknown or otherwise unloadable nodes, ignore them & keep processing.
• Nothing else needs to change: user configurations keep updating the rubberduck.config xml file.

[rkapka]

Can I be assigned to this?

[retailcoder]

@rkapka absolutely! I'm so sorry I didn't send you the team invite before!! I thought I had actually...

[rkapka]

In relation with the discussion in #3534, I am not sure how moving the default hotkeys into a .settings file will help with dependency injection circularity. When loading the default hotkeys, some component (most likely the DefaultHotkeys class) will need to have IEnumerable<CommandBase> injected to know what commands are available for hotkey binding at runtime.

I see another issue: serializing/deserializing the commands that were assigned to hotkeys. If I assign a CommandBase instance to a hotkeys' property, how to serialize it in the application/user settings? How about using the type of command, which will be a part of the setting name? Loading hotkeys would then require comparing that type name with elements from the injected CommandBase collection and assigning a matching command instance, if a match is found. Seems a bit dirty to me, but I can't think of anything better at the moment. Still, this will require injecting the commands, leading to the dreaded circularity error.

[MDoerner]

The way I see this, having the default hotkeys in the settings file allows to completely remove the hotkeys from the commands. Then only the hotkey bindings know about commands but commands have no idea about the existence of hotkeys.

[rkapka]

@MDoerner The latest version of the PR I mentioned works just like this. When creating the default hotkeys, a command is passed to the hotkey and the command itself knows nothing about the hotkey he's been bound to. But in order to get the collection of commands available for binding, I inject it into DefaultHotkeys. This causes an exception, because DefaultHotkeys is injected into HotkeyConfigProvider, which itself is injected into something else etc. In the PR's opening post I have described the dependency cycle causing the exception. This was prior to creating the DefaultHotkeys class, so there is another level of indirection at the moment, but the problem stays the same.

Am I missing something?

[Vogel612]

@rkapka The idea of introducing the .settings file is to make the concept of DefaultHotkeys obsolete

This means you don't have a circular dependency, because DefaultHotkeys doesn't exist. It's not code anymore, but data. And that data just needs to be processed correctly in the HotkeySettings (or it's provider, I forget which)

[Vogel612]

Follow-up discussion in chat

[rkapka]

First of all, many thanks for looking closer at the matter in chat.

From chat: He tried to inject the IEnumerable into the settings provider and ended up with the circular dependency above. The particular problem he described is that the TestExplorerViewModel takes an IGeneralConfigProvider, which always also contains the HotkeySettingProvider.

Exactly.

From chat: As long as there are views able to open the settings dialog, there will be a circle regarding the hotkeys. So, we will have to use property injection at some point to break the circle.
The property gets injected after objects have been created, so that everything works fine. We just have to find the correct place.

Are we talking about injecting the commands? I have been trying to do that previously, but without success. If I set up DefaultHotkeys like this:

public class DefaultHotkeys
{
    public IEnumerable<HotkeySetting> Hotkeys { get; }
    public IEnumerable<CommandBase> Commands { get; set; }

    public DefaultHotkeys()
    {
        // whatever
    }
}

and register hotkeys in this way:

container.Register(Component.For<DefaultHotkeys>()
                .OnCreate((kernel, item) => item.Commands = kernel.Resolve<IEnumerable<CommandBase>>())
                .LifestyleSingleton());

then I get this exception:

FATAL-;Rubberduck._Extension;Startup sequence threw an unexpected exception.;Castle.MicroKernel.ComponentNotFoundException: No component for supporting the service System.Collections.Generic.IEnumerable`1[[Rubberduck.UI.Command.CommandBase, Rubberduck, Version=2.1.6533.40782, Culture=neutral, PublicKeyToken=null]] was found
at Castle.MicroKernel.DefaultKernel.Castle.MicroKernel.IKernelInternal.Resolve(Type service, IDictionary arguments, IReleasePolicy policy)
at Castle.MicroKernel.DefaultKernel.ResolveT

[MDoerner]

First of all, I have not tried to figure out where the CW exception is coming from, so far.

Let me recapitulate where the circular dependency in PR #3534 is coming from. After that, I will pose some design/functionality questions to determine whether the settings file actually solves the problem. Finally, I would like to point out some options we have in case we want to take the route where the circular dependency cannot be avoided easily.

The Base Problem

The basic problem why constructor injecting the commands registered for CommandBase into the HotkeySettingsProvider, be it directly or indirectly via some DefaultHotkeySettings class, is that some of our commands open their corresponding windows. Thus, they have to know about these. In turn, some views allow to open the settings dialog, for which the corresponding view models need to know the general settings provider, which contains the hotkey settings provider. (I do not really understand why they need to new up the settings dialog themselves instead of using the corresponding command. However, that would not break the cycle anyway.)

Design/Functionality Decisions

The question I would like to raise here is:

"Does the HotkeySettingsProvider really need the commands?"

To put this into perspective: whoever creates the hotkey bindings definilty needs the commands since the hotkeys need to know the command to execute. However, in principle, the settings provider only needs the types or any other key identifying the commands. As we do no longer read the default hotkeys from the commands after this issue has been resolved, whether the HotkeySettingsProvider needs to know the commands loaded boils down to a simple design/functionality decision, in my opinion:

"Do we care whether there might be settings provided for commands that are not loaded or can we live with simply not binding the hotkey if the command is not loaded and still displaying the setting?"

For the next section, I will assume that we care; otherwise, there is no cycle anymore.

Breaking the Cycle

In this section, I would like to present two ways to break the cycle, based on the corresponding section of Dependency Injection in .NET.

The first possibility is a redesign of how we present the settings dialog. To truly break the cycle, we could employ an observer patter, or in other words an event-based approach. More precisely, we could inject some show settings broadcaster with a show settings event to which some presenter actually showing the dialog registers, That would invert one arrow in the cycle since the view models no longer depend on the presenter; they only depend on the broadcaster. Admittedly, this design is a bit unintuitive and confusing. Exactly because of this, this is not used to break the cycle in the WPF MVVM example in the book.

Option two is to use property injection. However, this is not as easy as just changing the registration code to use property injection. We truly have to inject after everything in the cycle is resolved. (OnCreate injects right after creating the object, which is too early.) Moreover, we have to make sure that nothing uses the information to be injected via a property in its constructor. Otherwise, it will not get the correct value.
In the book, the correct property injection is achieved by constructor injecting an abstract factory into the receiver of the dependency. Then, the factory is used to set the property right before it is needed for the first time.

[MDoerner]

I thought a bit about the problem with the dependency cycle again and another point at which we could break the cycle came to my mind: between the commands and the presenters.

In the particular example of RunAllTestsCommand, all the command does with the presenter is to show the window before running the tests. This is a bit strange from a design perspective since it pollutes the command with UI concerns while everything else deals with non-UI concerns.
Instead, there could be RunTestsRequested event somewhere, e.g. on the model, to which the presenter registers. That would be invoked by the command.
This would also remove the need to sometimes pass in null as the presenter when constructing the command (like in the view model).

[rkapka]

I am still not 100% sure that we can get rid of the dependency cycle without using techniques described in Breaking the Cycle section.

HotkeySettingsProvider creates HotkeySettings. Default hotkeys are a part of HotkeySettings, so the provider will have to deal with commands, be it directly or indirectly.

[MDoerner]

Let me explain why I think that the need break a cycle depends on whether we only want to provide settings for commands that are loaded or do not care.

Provided, we do not care that we might provide hotkey settings for commands that are not really loaded, the HotkeyProvider does not need to know which commands have been loaded. Neither the HotkeySettings nor the DefaultHotkeySettings is really a problem since both only need to know of an identifier identifying commands, i.e. the type or whatever identifier we decide to go with. They do not need to know the actual commands. (We cannot serialize them anyways.) The job do actually build Hotkey objects based on HotkeySettings objects should rather go to some HotkeyFactory, into which we constructor inject the commands.

[rkapka]

@MDoerner A few days ago I commited some code to #3534 dealing with things discussed here. Could you take a look at it? I would like to get some feedback before I start writing unit tests for all of this.

[retailcoder]

@rkapka I wonder.. @rubberduck203 confirmed earlier this week that the reason we hadn't gone with standard .net configuration settings was because we are a DLL project and apparently app.config files don't work when you're not an app. I'm confused now, can this work or not?

[Vogel612]

@retailcoder I'm pretty sure that while we can't necessarily use the standard .net configuration settings, it'd be a huge boon to RD to have something that has the same behaviour. If that means shipping a default configuration inside the .dll as binary, so be it.

If that means writing explicit code to merge our default configuration and a user-configuration, so be it. If app.config files don't work, we can still use alternative means of specifying a default configuration. IIUC the PR does that already, by encoding the default settings as XML inside a resource file.

[rkapka]

@retailcoder I investigated this matter a little bit and these are my thoughts:

The .settings file generates an XML file and a code-behind file. This code-behind file contains a DefaultSettingValueAttribute for every setting, where the value is a string holding the setting's XML. Even if RD is not able to read the app.config file (I don't see one in the bin folder), it will fall back to default settings, which are hard-coded in the code-behind file. While developing, any change to app.config displays a pop-up in VS prompting to update the .settings file, which in turn will regenerate the code-behind file and update the hard-coded values.

The code in my PR works. It loads the defaults and merges them with user-specific settings. I guess it just doesn't read the defaults from the app.config file.

I believe this StackOverflow thread might help. If I understand the top-voted answer correctly, if there is one RD DLL for all users of a machine, then they will have to share the defaults at the moment - but is that an issue?