[Issue #128] Configuration cleanup

Everything about development and the OpenMW source code.
Locked
User avatar
lgromanowski
Site Admin
Posts: 1193
Joined: 05 Aug 2011, 22:21
Location: Wroclaw, Poland
Contact:

[Issue #128] Configuration cleanup

Post by lgromanowski »

lgro wrote: Hi,
I have started work on configuration cleanup (Issue #128 in Redmine).
You can watch progress in [url=git://github.com/lgromanowski/openmw.git]git://github.com/lgromanowski/openmw.git[/url] "config_cleanup" branch.

Comments, suggestions, critics are welcome.

P.S I don't have an access to MacOS system / platform so if someone have it please verify
if this branch can compile and run without problems.
sir_herrbatka wrote: Great! :-)

Our OSX guy is corristo but He was silent recently.
Zini wrote: Didn't have the time to look at your changes more in detail, but I have two comments:

1. Please leave out the // ----- separators. No other part of our code base is using such a structure. While we aren't too strict about these issues, new code should generally try to follow our guidelines in the wiki and in cases where it doesn't say anything about the topic at question, new code should follow instead what has been established by the existing code.

2. Translation units should usually consist of a hpp and a cpp file. We aren't too strict about that either, but please avoid adding bulky function bodies to the hpp files.
lgro wrote:
Zini wrote:Didn't have the time to look at your changes more in detail, but I have two comments:

1. Please leave out the // ----- separators. No other part of our code base is using such a structure. While we aren't too strict about these issues, new code should generally try to follow our guidelines in the wiki and in cases where it doesn't say anything about the topic at question, new code should follow instead what has been established by the existing code.
Ok, I will remove that (It is old habit, and I often forgot about this in new code).
Zini wrote:2. Translation units should usually consist of a hpp and a cpp file. We aren't too strict about that either, but please avoid adding bulky function bodies to the hpp files.
Ok, I will move larger methods into cpp files.
lgro wrote:
sir_herrbatka wrote:Our OSX guy is corristo but He was silent recently.
Good to know, maybe he will appear and be able to check this :)
lgro wrote: Hi,
I've just pushed to github another part of code - changes:
- added Cfg::ConfigurationManager class
- small rewrite of main.cpp and engine.cpp files
- small bugfixes in Files::Path,
- added two missing *.cpp files: windowspath.cpp and linuxpath.cpp

Currently that code was tested on Linux only, but I will try to test it on Windows too (in next week).
pvdk wrote:
lgro wrote:
sir_herrbatka wrote:Our OSX guy is corristo but He was silent recently.
Good to know, maybe he will appear and be able to check this :)
He was replying to the launcher thread so consider him active.
Zini wrote: Sorry, but I don't agree with this commit. You are moving stuff around that is not related to the tasks. Adjusting whole source files to your personal preferences regarding code layout is not a good thing. Its okay to adjust non-compliant code according to the coding standards described in the wiki, but that's all.
btw. the command line option documentation you added is redundant. The code documents it well enough and the extra documentation only means we have to put additional work in keeping it in sync with the code.

Edit: Also, I see that there is now a configuration manager. What do we need that for? Configuration files were already handled properly (except for a small OS X problem that our Mac expert just had fixed). The task was meant as a small clean up: standardise on one type for path strings and move stuff out of main and engine. I am sorry, but I think you are on the wrong track.
lgro wrote:
Zini wrote:Sorry, but I don't agree with this commit. You are moving stuff around that is not related to the tasks. Adjusting whole source files to your personal preferences regarding code layout is not a good thing. Its okay to adjust non-compliant code according to the coding standards described in the wiki, but that's all.
OK, as you wish - I can revert change in engine.cpp/hpp which "move code around", but in my personal opinion putting class member variables on the top of class is really uncommon and makes that file harder to read.
Zini wrote:btw. the command line option documentation you added is redundant. The code documents it well enough and the extra documentation only means we have to put additional work in keeping it in sync with the code.
Sorry, but I don't agree with this. Program options shall be documented somehow in docs (or wiki) not only in code.
Zini wrote:
OK, as you wish - I can revert change in engine.cpp/hpp which "move code around", but in my personal opinion putting class member variables on the top of class is really uncommon and makes that file harder to read.
Well, that is just a personal opinion. There problem here is that your changes will make any merge for the engine file pure hell. If another person is making changes to the engine translation unit at the same time, I will have to examine every single line of it. If you only had changed what needed changing, such a merge would work most likely automatically.
Zini wrote:
Sorry, but I don't agree with this. Program options shall be documented somehow in docs (or wiki) not only in code.
Program options should be included in the end-user documentation. The comments in the code and the Doxygen documentation generated from it are not end user documentation. I guess the readme file that is currently written would be a good place for it. But simply copying the output of ./openmw --help should be enough.
lgro wrote: I've just pushed sources to github with reverted "move around" changes in engine.cpp/hpp and with removed options description from doxygen comment in main.cpp.
Zini wrote: Well, okay. Is this finished then?
lgro wrote:
Zini wrote:Well, okay. Is this finished then?
Yes, but I would like to check this code on Windows - I will try to find some time tomorrow and compile OpenMW on Windows.
Zini wrote: Better also check for OS X (corristo is our resident OS X expert).
Zini wrote: btw. could you merge in my master branch, please? There have been some changes (in the launcher) that probably require some adjustments in regard to this task.
lgro wrote:
Zini wrote:btw. could you merge in my master branch, please? There have been some changes (in the launcher) that probably require some adjustments in regard to this task.
Sure, I will do it this evening.
lgro wrote: I've just pushed rebased config_cleanup branch.
Locked