Configuration/Path Handling/Packaging

Everything about development and the OpenMW source code.
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Configuration/Path Handling/Packaging

Post by Zini »

I am making a new thread, because the old one was getting a bit crowded.

lgro is back and will working on the issue again (hopefully with the help of platform developers where needed).

Unfortunately the code in its current state is unusable. I probably shouldn't have merged it in the first place, but it had an important bug fix that was needed.

Let's start with components/files/*path.*:

Here I see several problems:

1. The terminology does not match what we use in the forum and elsewhere. Global is correct, but what is now called local we call user and what is now called runtime we call local.

2. Please don't call them config paths. There are not. There are fixed path, searched by OpenMW depending on the operating system. Just call them Local, User and Global paths. In fact, I think it would be a good idea to rename the files (and the class) to fixed_path (FixedPath).

3. Since these paths are fixed, there is no need for a set function. These should be removed.

4. Now the data path is something complete different. First we don't have global or runtime data paths (to stick with the terminology currently used in the code): We have a local data path, but it is not related to the local (config) path. Data paths are determined by the openmw.cfg files found in the fixed file locations and by the command line options.

What we have is one local data path and zero or more non-local data paths.
The local data path usually points to User Path/data (I am using the correct terminology here, not the one in the code). The non-local data paths will be spread out over the whole file system.

In most cases the local data path will be no different from the other data paths. In fact it will receive special treatment only by the editor.
For OpenMW there is simply a sequence of data paths with descending priorities, with the local data path listed first.


Now to configurationmanager.*. I suggest the following changes:

First move it to files. Really no point in having a separate component for it.

Also remove all fixed_path functions from it. There is no point in routing these through the configuration manager.

Then the configuration manager should implement data path handling (without making use of files/*path.*.

Also the usage of configuration manager in the rest of OpenMW should be reviewed. I think the correct usage is to create one instance in Engine and pass a const reference to it to all subsystems that need to make use of it.


Edit: I just noticed, that Path's functions are not static any more, which means we need an instance of it. I suggest to make the (Fixed)Path instance a private member of the configuration manager and then keep the get functions for fixed paths (after renaming).
User avatar
lgromanowski
Site Admin
Posts: 1193
Joined: 05 Aug 2011, 22:21
Location: Wroclaw, Poland
Contact:

Re: Configuration/Path Handling/Packaging

Post by lgromanowski »

Zini wrote:I am making a new thread, because the old one was getting a bit crowded.

lgro is back and will working on the issue again (hopefully with the help of platform developers where needed).

Unfortunately the code in its current state is unusable. I probably shouldn't have merged it in the first place, but it had an important bug fix that was needed.

Let's start with components/files/*path.*:

Here I see several problems:
Hi,
please find my comments bellow:
Zini wrote: 1. The terminology does not match what we use in the forum and elsewhere. Global is correct, but what is now called local we call user and what is now called runtime we call local.
I used wrong naming for them, I will change it to the one you proposed.
Zini wrote: 2. Please don't call them config paths. There are not. There are fixed path, searched by OpenMW depending on the operating system. Just call them Local, User and Global paths. In fact, I think it would be a good idea to rename the files (and the class) to fixed_path (FixedPath).
In XDG Base directory specification there are two important (from our point of view) paths, the one where configuration files should be placed ($XDG_CONFIG_HOME) and second one where user/application data should be placed ($XDG_DATA_HOME). Similar paths exist for Windows and MacOS. Thats why I named "config" and "data" paths/methods in *path.hpp files, but I don't mind to change it if you want.
Zini wrote: 3. Since these paths are fixed, there is no need for a set function. These should be removed.
OK, I will remove that methods.
Zini wrote: Now to configurationmanager.*. I suggest the following changes:

First move it to files. Really no point in having a separate component for it.
OK, it will be moved.
Also remove all fixed_path functions from it. There is no point in routing these through the configuration manager.

Then the configuration manager should implement data path handling (without making use of files/*path.*.
I don't understand why. This design separates path "algorithms" used by different systems and avoid some #ifdef mess. If we add support for another system then I think it would be easier to add it in separate file and add just one #ifdef in path.hpp file. I think if I move content of *path.cpp/hpp files into ConfigManager it will be messy.
Also the usage of configuration manager in the rest of OpenMW should be reviewed. I think the correct usage is to create one instance in Engine and pass a const reference to it to all subsystems that need to make use of it.
There is one instance - it is created in main.cpp (because of commandline argument processing there), Engine class have reference to CfgManager as a member.

Should I move CfgMgr instance from main.cpp to Engine class?
Zini wrote: Edit: I just noticed, that Path's functions are not static any more, which means we need an instance of it. I suggest to make the (Fixed)Path instance a private member of the configuration manager and then keep the get functions for fixed paths (after renaming).
(Fixed)Path as a private member of ConfigurationManager - it is already there, so I think the only changes would be method renaming and removing setters.
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Re: Configuration/Path Handling/Packaging

Post by Zini »

In XDG Base directory specification there are two important (from our point of view) paths, the one where configuration files should be placed ($XDG_CONFIG_HOME) and second one where user/application data should be placed ($XDG_DATA_HOME). Similar paths exist for Windows and MacOS. Thats why I named "config" and "data" paths/methods in *path.hpp files, but I don't mind to change it if you want.
Well, we store config files there. But also a lot of other stuff. Log files, possible additional data directories and such.
I don't understand why. This design separates path "algorithms" used by different systems and avoid some #ifdef mess. If we add support for another system then I think it would be easier to add it in separate file and add just one #ifdef in path.hpp file. I think if I move content of *path.cpp/hpp files into ConfigManager it will be messy.
See my edit in the original post. Obviously when we have Path as a private menber of the config manager, we need to expose the get functions as it is currently done. This paragraph was written before I noticed my mistake.

Another edit: The above is meant for the original Path functions (user, global, local). The data path is an entirely different topic. It should definitely be moved to the configuration manager. There is nothing system specific in it.
There is one instance - it is created in main.cpp (because of commandline argument processing there), Engine class have reference to CfgManager as a member.

Should I move CfgMgr instance from main.cpp to Engine class?
main.cpp works too. But in your absence some people used the conguration manager without fully understanding how it was to be used (possible including me, I don't remember). Please make sure we didn't mess up there.
(Fixed)Path as a private member of ConfigurationManager - it is already there, so I think the only changes would be method renaming and removing setters.
Alright. That escaped me.
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Re: Configuration/Path Handling/Packaging

Post by Zini »

I thought a bit more about the next step and expanded my idea mentioned in the other thread.

So we need to be able to set data paths to specific locations predefined by the system/external programs (example the MW installation directory).

I suggest to add another path expansion step when a data path is passed into the Configuration manager (the actual translation function is probably best implemented in Path).

This expansion step should look for special tokens and replace them.

Examples:

Global:Data files (This would be expanded to the subdirectory "Data files" in the global location)
MW:Data files (This would expanded to the subdirectory "Data files" in the MW installation directory).

The values for the tokens Global, Local and User you can obviously read from the Path class.
The value for the token MW you can read from a registry key on Windows (I think) or by querying WINE for this registry key (on Linux and OS X).

A few things to consider:
- A token might not be available (e.g. MW not installed or WINE not installed). In this case the data path is invalid and should be silently removed from the list by the configuration manager.
- The directory specified by a data path might not exist. This also invalidates the data path. The only special case here I am not sure about is the local data path. The directory might not exist, in which case the editor needs to create it instead of discarding the path. But we can handle this special case later.

Actually I think the x:y syntax for tokens is probably not very good. You should choose something that not collides with the existing rules for constructing path strings on all three supported platforms.
User avatar
Ace (SWE)
Posts: 887
Joined: 15 Aug 2011, 14:56

Re: Configuration/Path Handling/Packaging

Post by Ace (SWE) »

I don't know if it helps but I wrote a quick piece of code for retrieving the Morrowind install path from the windows registry, it's on GitHub even though I failed a bit with the branch (forgot to switch to it before committing).
It works on my 64-bit install and it should work on 32-bit windows too.
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Re: Configuration/Path Handling/Packaging

Post by Zini »

Looks mostly reasonable. I would make it a private function, since it will be used only internally by the path expansion function.

There is another point I have to criticise though: new[] is bad. You shouldn't use it at all in C++ (expect for some very specific cases related to very low level memory handling techniques; e.g. when you are writing an implementation of the STL).

You have seen the reason yourself. new[] is error prone. You forgot the delete. If you had used a std::vector instead, this problem could have been avoided completely.
User avatar
Ace (SWE)
Posts: 887
Joined: 15 Aug 2011, 14:56

Re: Configuration/Path Handling/Packaging

Post by Ace (SWE) »

I wasn't aware std::vectors could be used for functions requiring an array (pointer), how would one do that?

Edit:
Never mind, modified it to work with a vector instead.
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Re: Configuration/Path Handling/Packaging

Post by Zini »

Thanks!

On a second thought there is probably also a valid implementation path with this function being public. Let's leave this decision to lgro.
swick
Posts: 96
Joined: 06 Aug 2011, 13:00

Re: Configuration/Path Handling/Packaging

Post by swick »

Did the same thing for linux. It should find the "Installed Path" in wine:
https://github.com/swick/openmw/commit/ ... 6e7423f180
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Re: Configuration/Path Handling/Packaging

Post by Zini »

Nice. Now we only need a solution for WINE under OS X. Or would that be the same as under Linux?
Post Reply