patch for rearranging global options

Discuss development issues and submit patches here
Post Reply
ajs
Posts: 409
Joined: Mon Jul 05, 2010 6:37 pm

patch for rearranging global options

Post by ajs »

Hi David,
this patch doesn't add any new functionality to E++, but it re-arranges the way global settings are managed.
It adds a new "CGlobalSettings" singleton class in the Helper project, which is meant to be the container of all global settings which needs to be shared between E++, MyTreeView, CFolderView and whatever else. In this way we avoid to duplicate the code needed for each option in different part of the project.
I have already re-arranged the global options of:
1) Options -> Files and Folders
2) Options -> Default settings

The original patch 13 is superseded and has been replaced by patch 14.
Therefore the original file patch_13__v161 has been removed


Patch removed. See comments in following posts for more details.
Last edited by ajs on Sat Jan 08, 2011 11:33 pm, edited 1 time in total.
ajs
Posts: 409
Joined: Mon Jul 05, 2010 6:37 pm

Re: patch for rearranging global options

Post by ajs »

updated today
David Erceg
Site Admin
Posts: 933
Joined: Sat Apr 18, 2009 1:46 am

Re: patch for rearranging global options

Post by David Erceg »

I'm not so sure about this patch. I've thought quite a bit in the past about how best to manage settings. I definitely understand that passing configuration information between objects can become quite bad in terms of maintenance, but I don't know that having a singleton settings class will solve those problems.

At the moment, all projects except Explorer++ are explicitly designed to be independent of each other. They all rely on the Helper project, but the Helper project is more or less a loose collection of miscellaneous functionality. If there was going to be a singleton settings class, it would have to be pushed into its own project (it definitely doesn't belong in the Helper project).

In my mind it also introduces a greater dependency between each of the projects. Explorerplusplus currently holds the settings it needs to initialize all the other objects within the application. This is ok, since Explorerplusplus already depends on all the other projects (and unlike the other projects, it's not meant to be reusable in any way). A settings class would naturally have to sit near the bottom of the class heirarchy, and it would be the one holding/managing the settings. Now, there would be an indirect dependency between all projects/classes and the settings class.

In terms of variable initialization, I think this is something that belongs in the Explorerpluplus class (rather than in the constructor of CGlobalSettings). Essentially, Explorerplusplus manages objects created from the other projects, so it's the one that gets to decide on the initial value of things.

Having a singleton also means any lower layer object could change any global setting at will, which shouldn't be allowed at all. There's also the issue that the lower layer objects still need to be notified in some way when a setting changes. With a singleton settings class, it's possible that a setting could be changed, and the lower layer object not notified (meaning it can't update itself). Currently, the only way to change a setting is to call Set*, which ensures the object is always aware of the change (since there's no other way of doing it).

Unfortunately, I just don't think switching to a singleton class is a good idea. If you have a specific argument in favour of it, I'm more than happy to listen, but I don't think the benefits outweigh the cost.
ajs
Posts: 409
Joined: Mon Jul 05, 2010 6:37 pm

Re: patch for rearranging global options

Post by ajs »

Hi David,
I thought quite a lot before engaging the development of a global settings class.
First let me make an example: take any option shared by E++ and MyTreeView. At the moment the option is defined in Explorer++, then it it re-defined in the CFolderView and finally in the CMytreeView class, together with all the methods for setting/getting the value (an example could be the HideRecycleBin variable). This means that anytime we need a common option between different projects, we need to duplicate code and this could lead to mistakes when the options are many (not to forget the duplication of code).
Another example is any global option shared by any tab and not individually settable (like ShowGridLines for example). Once again there is duplication of code and variables.
Because of this reasons I started to think of a class which would collect and group the program global settings together: no more duplication of code and a single point in the code where we need to make changes if we have to (and therefore less possibility to make mistakes).
In terms of "project indipendence", I understand your comments and I partially agree with those. Nevertheless we can't ignore the fact that the main projects (E++, MyTreeView and ShellBrowser are strictly interacting with each other (for example, just to name one, selecting a folder in the TreeView will cause the listview to change). So there are two ways to handle the global settings:
1) each project has his own (duplicated) settings. Whenever we change one, we need to remember to change the code in every project, which IMHO is a more error-prone coding.
2) we centralize the management of the global settings in one point. The code (as I wrote it in the last couple of days) is not optimum, and I agree with most of your comments. To overcome some of the weak points you mentioned, we could do the following changes:
A) move the class from the Helper project to the E++ project. This let E++ be in charge of the global settings
B) introduce Get/Set methods. The Get methods would be public and accessible from each project, while the Set methods would be private and accessible only from Explorer++ and the (XML)Settings classes (to set them after reading the settings from the (XML)registry).
In terms of notification of changes, that wouldn't be a big problem. Currently (without the CGlobalSettings class), E++ pushes the new values to the different projects whenever it is needed. Using CGlobalSettings, a project can pull the values whenever it needs and E++ can still notify changes when necessary through method calls.
C) the CGlobalSettings class single instance would be created and hold by E++, while the other projects still need to have read access to the class's Get methods. We can add a static method to E++ for that.

If the design goal is for each project (excluding E++) to be fully independent and reusable, then we should not even consider the CGlobalSettings class. Nevertheless I don't think that such goal could be 100% possible, because E++ would have to get the status of each project continuously (i.e. no notifications from other projects would be allowed towards E++).

Anyhow you are the main developer, so the decision of "which way to go" is yours. If you prefer to stay with the actual design, I will remove patch 14 and I will continue without that.
David Erceg
Site Admin
Posts: 933
Joined: Sat Apr 18, 2009 1:46 am

Re: patch for rearranging global options

Post by David Erceg »

Thanks for the reply Michael, you certainly raise some good points.

In terms of project independence, I'll briefly point out that MyTreeView and ShellBrowser don't directly interact in any way. Explorerplusplus manages all interactions. In the specific case of the listview folder changing when the treeview folder changes, this is performed by Explorerplusplus within TreeViewHandler.cpp (essentially the treeview and its parent are both subclassed).

I've been thinking about the problem a bit more, and I think there are two basic issues:
1. Duplicated code/variables.
2. Management of the global state.

In my mind #1 isn't such a big deal. The way I view it is that each project/class has its own settings which it stores and manages. The class then provides get/set methods to allow its owner to change parts of its internal state. The settings are duplicated within Explorerplusplus because it needs the settings to be persistent and global (of course none of the other projects/classes are aware of this). So there is some duplication, but it helps to separate the projects, and I don't think there's too much of an overhead (realistically, there aren't a huge number of 'global' settings). I guess my main point would be that each of the projects should remain as independent as possible.

Point #2 includes how easy it is to determine what settings projects are exporting (should be possible simply by looking at their get/set methods), and how well those settings are managed by Explorerplusplus. I think the management of the settings by Explorerplusplus may be one of the main problems right now. Global settings need to be saved/loaded/initialized as well as passed to tabs/the treeview when they are created, and they need to be updated within the Options dialog. It may be worth having a class that centralizes all of that within the Explorer++ project.

In terms of independence and notifications from other projects towards E++, it would be possible (and in fact it's already done in several places). It's just that the notifications have to be defined by the child project, rather than the parent. The notification could take the form of a callback, or a window message. For example, MyTreeView sends out a message to its parent when it gains focus. In some cases the child may not even be directly involved (e.g. window subclassing). None of this breaks the independence of the project though, because the child sends out the notifications regardless of who the parent is. The notifications that are sent are almost entirely dependent on what information Explorerplusplus needed at the time, but it's important to realise that the notifications are usually quite useful anyway, and they are managed completely by the child project.

Overall, I'm almost sold on the idea of passing an interface or object to each of the projects that contains their global settings, but ultimately I think the cost of the extra dependency is too great. MyTreeView, for example, could be used in another project right now, provided that Helper is available. Having a global settings object would mean that the owner would have to create that first (since MyTreeView would not longer be able to manage itself fully).

I think there is a lot of value in trying to centralize the global settings code within the Explorer++ project though. So, we could have a class that initializes the settings correctly, that is able to forward them off to the treeview/tabs when necessary, etc. Unfortunately, I don't think I'll go with the singleton code at this stage.

Anyway, none of the above is meant to be critical, I actually enjoy having the opportunity to discuss some aspects of the design so thoroughly :)
ajs
Posts: 409
Joined: Mon Jul 05, 2010 6:37 pm

Re: patch for rearranging global options

Post by ajs »

Ok David, no problem. After reading your comments, I suggest we proceed this way:
1) I will remove patch_14
2) some time in the (near) future I will figure out how to design a class within the E++ project which will handle the program settings. The class would be responsible for loading/saving/managing the settings. The interaction between E++ and the other projects would remain unchanged (i.e. does not introduce any new dependency).
3) before starting implementation, let's discuss the design again.
David Erceg
Site Admin
Posts: 933
Joined: Sat Apr 18, 2009 1:46 am

Re: patch for rearranging global options

Post by David Erceg »

Sounds good.
Post Reply