Reduce the scope of the Settings Dialog

[CharliePoole]

The Settings Dialog provides a way to record user decisions about the functioning of TestCentric, global decisions, which are used for every project. As discussed in #1412, we want to radically reduce the size of this dialog. ..

• Some settings will be removed because we don't actually support them any longer.
• Other settings are no longer needed because NUnit provides satisfactory default values.
• Still other settings are accommodated in other places in the GUI and are redundant.

We want to end up with a Settings Dialog which has only...

1. A small number of settings used only to initialize new projects.
2. A small number of GUI-related settings, which are not convenient to accommodate elsewhere and are most likely to be generally desirable, rather than varying by project.

Settings only used to initialize new projects should be on a separate page and clearly indicated as such.

Specific decisions will be made as the code is written and reviewed in the PR.

[rowo360]

I would like to work on this issue and have therefore assigned it to myself. I think I will split the task into several commits (maybe one commit per setting page) to keep the overview. As a preparation I read your description listing every setting (link), looked up some code parts and agree with your proposals. Here are only some additional comments:

• Whenever the configuration of a setting is removed from the settings dialog and it cannot be set somewhere else, I will also remove the setting from the application setting.
• Visibility of statusbar: The statusbar will be always visible afterwards. The user cannot toggle show/hide of the statusbase anywhere else (for example main menu).
• Recent files - check that file exists before listing: This setting is set by the SettingDialog, but it is not used anywhere. So, the 'recent file list' contain all files regardless if a file exists or not. I propose to keep it as it is now.
  Or in other words, I propose to set the new, fixed default value to false.
• Recent files - MaxFiles: Maybe this default value of 24 is quite high? I need to see this full list in action and maybe reduce it to 20.
• Automatic Reload - rerun when reloading: I propose to keep this setting. If it works as desired, then this “continuous testing” feature is really helpful. However, I propose to move it in the application setting to a different category. Currently, it's located in the EngineSettings - and, well I think that's not the best location. Hopefully there's a better category.

[CharliePoole]

This looks great to me as a plan. I like the idea of multiple commits. Be cautious in removing settings, especially settings we have no control over. But I may not understand what you mean about "remove the setting from the application setting" so I'll wait and see in the PRs.

[rowo360]

So, first part is done and the first PR is merged. Let's focus next on the topic "User default for new projects" (link)

Here's a list of all kind of options to control the visual aspects of the tree (partially from our application settings and partially from the VisualState file). Theoretically, a user could prefer any of these settings as their default.

• Show checkbox
• DisplayFormat (NUnit or TestList)
• GroupBy (NoGrouping, Category, Duration,...)
• ShowNamespace
• Show duration (test duration is shown in brackets in the tree node)
• Sort mode (Alphabetical or test duration)
• Sort direction (ascending or descending)

I suggest that we only offer these settings as user-configurable default values:

• Show checkbox
• DisplayFormat (NUnit or TestList)
• ShowNamespace

At first, I still had GroupBy in the list, but I actually think that the fixed default value ‘NoGrouping’ will always be a good fit. You might be surprised about the presence of 'ShowNamespace' - if a user disable this setting, he will get the look&feel similar to the FixtureList as his default. So, I thought it might be worth to offer this one as a user-configurable default value as well.

I suggest to add the prefix 'Initial' to all of these settings (e.g. InitialShowCheckBoxes) to make their purpose clear and to avoid mix-up with the currently applied value in the UI. In the Settings dialog, I would add a label explaining that these settings only apply to new projects.

[rowo360]

So far, so good - but looking at the details, it's unclear for me how to design/implement it best. Let's look at the setting 'DisplayFormat' as an example. The TestCentricMainView provides a ToolStripElement for this setting and the TestCentricPresenter enable/disable this element and handle the SelectionChanged event of this element. Now, the important part, this eventhandler in TestCentricPresenter only sets the DisplayFormat setting - that's all. Other presenter classes (TreeViewPresenter) listen to setting changes and update their UI accordingly (for example: rebuild the tree).

But will we even have a DisplayFormat setting in the future? Or will we have only one setting, InitialDisplayFormat instead? (which is shown in the settings dialog). Somehow it feels wrong to continue managing a DisplayFormat setting. A setting which is stored in the user.config file. Instead I've the impression that we simply need a property DisplayFormat which manages the current state. When creating/loading a new tree, the value of this property is either initialized from the InitialDisplayFormat setting or the value from the VisualState file. This new property could be part of the TreeViewPresenter or the DisplayStrategy or...

But this idea creates a new challenge:
Without any setting, there won't be any SettingChanged event and the glue between the EventHandler in class TestCentricPresenter and the SettingChanged handler in class TreeViewPresenter falls apart. Instead, I would prefer to process the SelectionChanged event of the ToolStripElement directly in class TreeViewPresenter. Because the value of the selection only affects the tree and not the mainview. Unfortunately, we have never done anything like this before: every presenter class has its own view class. In order to handle the SelectionChanged event in class TreeViewPresenter, I would have to pass in the ToolStripElement form the mainview into the TreeViewPresenter (as a ISelection interface - in constructor or separate Init(...) method).

I would really like to do all of that, but it also sounds like it would involve quite a lot of changes. That's why I wanted to hear your opinion on the matter. Is it worth changing all of this, or should we just continue to use (misuse) a ‘DisplayFormat’ setting?

[CharliePoole]

I suggest that we only offer these settings as user-configurable default values:

Show checkbox
DisplayFormat (NUnit or TestList)
ShowNamespace

At first, I still had GroupBy in the list, but I actually think that the fixed default value ‘NoGrouping’ will always be a good fit. You might be surprised about the presence of 'ShowNamespace' - if a user disable this setting, he will get the look&feel similar to the FixtureList as his default. So, I thought it might be worth to offer this one as a user-configurable default value as well.

I see your point regarding GroupBy but we already decided that the absence of FixtureList is not a big loss. I'd rather just keep the first two. It's trivial for the user to change the setting and from then on it's saved for them in the visual state. Of course, we could say that about all these settings so we could even consider leaving this feature out for the initial release.

To your second comment - about implementation problems - I'll have to give it some thought before answering. In the morning, that is...

[rowo360]

Here is a suggestion for the implementation that does not turn everything upside down. It's more of a gradual evolution, and we can stop at different steps. I like that kind of approach much more.

1. The setting class TestTreeSettings will contain properties for the initial default values, but also for the actual values. For example, InitialShowCheckBoxes and ActualShowCheckBoxed, InitialDisplayFormat and ActualDisplayFormat or ActualGroupBy (supports no initial default value).
   While the properties names 'Initial...' are real setting property in the sense of the ApplicationSettingsBase class with these attributes:

        [UserScopedSetting]
        [DefaultSettingValue("false")]
        public bool InitialShowCheckBoxes

the properties named 'Actual...' are simple c# properties without these attributes. This means, they will not be stored in the user.config file - that's exactly what I wanted. However, I want that any changes to these simple c# properties still invokes the SettingChanged event. That will not work out-of-the-box, but I think that can be done without much effort (those changes would only be located in the settings classes). The great thing is that all clients of this setting can continue to work as before (set/get setting or listen to SettingChanged event).

2. This step is only a minor modification: in step 1 the class TestTreeSettings contains a mixture of real settings and actual values. Maybe it's better to separate them into separate classes: the class TestTreeSettings continues to manage the real settings. But we introduce a new class TestTreeConfiguration (we can discuss the naming - I just want to avoid the term setting for now) and this class contains all the actual values. However, I would still propose to keep this new class accessible under the Model.Settings properties. The SettingChanged event is still supported. For example, clients can access these properties by:
   bool showCheckBox = Model.Settings.Gui.TestTreeConfiguration.ActualShowCheckBox

3. The class TestTreeConfiguration does not quite fit into these settings classes. It manages only some properties which are not persisted at all. So we could also remove them from the settings and manage them as an own entity. For the sake of simplicity I propose to aggregate the instance at the TestModel:
   bool showCheckBox = Model.TestTreeConfiguration.ActualShowCheckBox

The class still provides a Changed event - I would call it PropertyChanged instead of SettingChanged, now. Again all clients working with the actual tree configuration need to be adapted - but the nice thing is, that these client changes are really simple changes so far - there are no conceptual changes - there's still a Changed event!

4. The last step that occurred to me was that we could also move the new class TestTreeConfiguration from assembly Gui.Model to Gui. That class manages the actual configuration of the tree - so maybe it fit better to the GUI.
   Unfortunately this step introduces new challenges, so I'm bit cautious about it. Nonetheless, I write down my current considerations:
   We could create an instance of TestTreeConfiguration in AppEntry and pass in this instance to class TestCentricPresenter and TreeViewPresenter. So, one class can continue to handle the UI events and set a property and the other class can continue to handle the PropertyChanged event of the TestTreeConfiguration class - so, we don't need to change anything in the general UI concept what I mentioned in the previous comment and I'm glad about it!
   The remaining challenge is that the TestModel is currently omnipresent in all the UI classes. So, currently the settings can be accessed quickly TestModel.Settings... to retrieve any setting value. However now, we would always have to provide access to the TestTreeConfiguration class - for example by passing the TestTreeConfiguration instance from the two presenter classes into any relevant class. Maybe that sounds not to be a complex task, but I expect quite a huge number of changes.

Overall, I am glad about this step-by-step approach which seems much easier to me. What is your opinion about it and do you prefer to continue to a certain step for the start? Or do you have considerations in a complete different direction? 😄

[rowo360]

Maybe we can have a quick look to these two settings:

• ShowNamespace
• ShowTestDuration

Do we want to keep them as a setting? We already agreed on to not support any User default for them, but what about a setting in general? Such kind of setting which is implicitly set by checking/unchecking the associated menu item.

I suggest to remove ShowNamespace as a setting. The value is already saved in the VisualState file. So, once the user set the value, the value will be restored whenever the user reloads his project/test assembly. If he creates a new project, the fixed default value (true - in this case) is applied. That's all fine from my point of view. (Ok, if it's not setting anymore, it must be something different - see my proposal in previous comment).

Regarding the ShowTestDuration I propose to keep it as a setting. So, once the user activate it, the option will be applied to all his projects (new ones and old ones). It is currently not saved in the VisualState file and I would keep it that way.

Do you agree with these proposals or is it confusing for the user?

[CharliePoole]

I'm a little confused... if we decided (as I thought) that the user would have minimal control over initial settings (i.e. you suggested three settings, I prefer to see two) What is the purpose of this large set of "InitialSettings"?

If you are merely trying to gather them in one place, that's understandable, but you seem to be allowing the user to change all the initial settings. Have I misunderstood?

I venture to say that if I'm confused, at least a few users may be confused too. :-)

If we do set up this class or classes, I prefer separate ones. This is similar to the difference between a Setting and a SettingDefinition, which we previously had in the engine and is now in NUnit's engine api. The key difference is that definitions have defaults, while settings have actual values.

So, in summary, I don't want to give users more control over initial values than we have to. It multiplies the things we have to test. From experience, anything we give in the first release will never be removed. But if we don't give some feature, it's always possible to add. So putting all our defaults in one place makes sense to me, just not exposing them.

By the way, issue #1414 is changing a lot of code. So while the design of for this issue doesn't conflict with any of it, there may be a lot of conflicts in the implementations after merging.

[CharliePoole]

@rowo360 Maybe one way to clarify this is to start with the UI you plan.