-----Ursprungliche Nachricht----- Von: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] Auftrag von Yann Dirson Gesendet: Mittwoch, 30. November 2005 01:24 An: [email protected] Betreff: [Wesnoth-dev] About changes, and keeping the dependency graph clean
>Despite my occasional efforts to cleanup the dependency graph, I have >to say it does not get much better in the long run. I guess it mostly >have to do with communication, and agreeing about what each file is >all about. I wholeheartedly agree with this. Communication is really important in order to avoid messing up the code. But i think something even more important is having a "big picture" of what the code and its dependencies should look like. There is some general ideas (including a proposal of me on the forum in "Coders Corner" some time ago), but nothing that everybody really commited to. I also think that the code has reached dimensions that make more organization become a necessity rather than convenience. I consider myself quite good at digging into others code (i have to do that quite often) and although i did not grew up with C++ in the first place (more like Java/C#), i still have more trouble locating functionality and understanding how the code is linked together than i should. Having no "big picture" introduces another problem: Nobody can really tell what architecture is supposed to look like and where functionality is best put into. I asked that a couple of times and there is some hint now and then but the general tendency is more like "Go ahead and get it in, then we see if it works out well". r8785 was my first patch (and np yann, i don't take your comments as personal bashing). When i made that changes to config.cpp i was astonished that there were almost no includes in there, which is very untypical for a wesnoth source file. I got suspicious about that, but not enough to make me stop. My goal was to change config.cpp into providing an interface to convert wml into core classes like (unit, team etc). Well, not completely of course but at least the parts i needed at that moment. I even asked on irc about that but did not get an answer on it so i put it in. Oops! That broke about everything beside the client :). So i changed that, created config_adapter as a bridge between wml and core classes and restored config.cpp to what it was (r8797, r8798). Therefore i hope that dependency is not there anymore (if it still is, it is not necessary and we should get rid of that asap). Can such a thing be avoided? Sometimes yes, sometimes no. Everybody knows that and i am very thankful that you helped me get along to solve the problem (instead of kicking me out or something ;). And yes, trunk has to be broken sometimes to try things out. But (and here we return to the topic of yanns Mail): It should not be done more often then necessary. I am getting slowly more familiar with the code but many decisions where to put a new method are still based on guesswork rather than knowledge. So what can we do about this? 1. I would really really love to have that "big picture". But that - needs a lot of discussion in advance - needs careful planing afterwards - needs a strategy how to get from the current mess into an organized state again - has to be made known to and commited by everybody that touches src 2. I really really would love to see that dependency graph. Since i am on windows, chances are bad at the moment, but maybe a graphics file (gif?) every now and then would be very helpful. If there is any chance to get that in the wiki i strongly second doing that. Some more opinions of mine: 3. I am a strong supporter of the projects underlying technique of extreme programming. But i also think that some essential things are missing at the moment: We don't have tests (that is automated test code like unit tests). They would show us immediately, if things get broken or not. I have a very ambiguous opinion on unit tests, since they can easily become a project of itself. But we should at least consider to have tests for some important core functionality. One consequence of missing tests is that we are afraid to do bigger reworks of the code, even if they are done in small steps. XP is also about pair programming and there is one big reason for that: Communication! Now pair programming for wesnoth is of course a problem but communication is not. 4. As i said earlier, better organizing the code turns out to be a necessity IMHO. I go one step further and say: In a not too far away future, if we don't get the code organized better, many changes will break more things than they fix. This will become worse over time. So i make a proposal here: Every patch, that changes essential parts of the code, has to be sent to yann first to be approved. This of course only under the preassumption that yann finds enough time to not delay the development substantially. But i don't think, that those kind of patches will appear very often. As a first approach, we can define essential as changing includes of a source file. 5. We need someone to coordinate the efforts of getting the "big picture". This is a perfectly suited task for the "code leader", that was proposed by Dave lately. It doesn't mean he has to be doing all the work himself, he could as well delegate it. But he takes over responsibility and "keeps things going". So, to answer your questions: Having the dependency tree available helps a lot, commenting source code does so as well. But IMHO it won't be enough. We need coordination regarding essential changes as i outlined above. > - does replay_controller really need that much #includes ? I think so, yes. It is a typical controller, a link between gui and core functionality. As such it needs to "know" almost everything. But it is also one of maybe two controllers of that kind. The other possible candidate is playlevel/playturn which is separate from replay_controller at the moment. It should be integrated with it though, since many functionalities are shared. I plan to move play_level into a class, similar to replay_controller, and extract all the common functionality into a base class. I did not do that yet since it is very likely to break some things. Not as much as replay_controller, because i know the code quite a bit now, but still there is a risk. So i did not dare to touch that yet. And it is such a severe change, that i would like to have some discussion about it first. >At the same time I noted a couple of other changes which I do not think >are necessary, and indeed only break the consistency: > >+++ src/widgets/button.cpp (revision 8785) >[...] > #include "button.hpp" >-#include "../font.hpp" >+#include "font.hpp" > #include "../image.hpp" > #include "../log.hpp" > >That is, if this change was necessary on some platform, I'd rather >have a look at why this change was necessary - and, also I'd like to >encourage everyone not already doing that, to carefully review the >"svn diff" before committing, to spot such things, which are much more >apparent in the diff than in the plain files. If i understand this right, i just adapted the path to font.hpp. Shouldn't break anything or make things more complicated. Btw, there is still an open issue about relative vs absolute paths for header files. Most of them are absolute (originating from src-directory), but there are some exceptions, mainly within the gui widget files. We should have a decision about it and then clean up the paths accordingly. Ah, enough talking. Now it's your turn to comment on that. I am glad i don't have to do all these decisions :P. I am still experiencing "whelp protection", you know ;-) Kind regards Jorg Hinrichs
