[Wesnoth-dev] About changes, and keeping the dependency graph clean

[Yann Dirson]

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.

So let's start with the problem: we have many source files, which all
depend on many others.  So much that for example the editor has to
include network code, just for the sake of linking the executable.  My
quest is to get rid of this problem, by turning as much as possible
the dependency graph into a tree.

I have written a tool for this [1], and it is even hooked in the main
Makefile: "make wesnoth-deps.ps" will generate a graph from the
current code (note: for various reasons, including me being a lazy
bastard, you have to "rm wesnoth-deps.dot" to be able to rebuild the
graph).  Note that the tool most probably does not work on windows
yet, although making it more portable is on my todo list.

To get an idea, some relatively outdated graphs[2] are also available,
as an example for those of you who never saw them.



Now I realize there is something we should probably agree on, since
many of my cleanups end up being countered by subsequent commits -
that is, what is the scope of each module (.cpp + .hpp).

For example, till now I considered rules such as the following:

- there are core low-level stuff (eg. wml handling), core game
facilities (eg. unit types) building upon low-level stuff, core video
stuff (eg. fonts), widgets building upon core video, dialogs above
that, and higher level blocks tieing all together (eg. the game
client), to name a few

- the idea is that a low-level block should not depend on a
higher-level one: this is precisely what causes cycles, and thus
problems


Now to some of the current problem (I only looked in details at r8785,
since it was precisely introducing one annoying dependency I was
tracking, but finally turned out to introduce many, trigering this
mail, which should not be mistaken for a personal attack, or maybe
just against myself, who has a nasty tendency of doing the deps
cleanup without too much communicating ;):

- "config" is the home of wml, ie. really low-level.  I see 
that it now depends on gamestatus, preferences, map, and units, all of
which are definitely higher-level building blocks: surely the
functions introduced in that commit do not belong to this file

- I have as much as possible tried to turn "preferences" into just a
"preferences container", moving interactions with the gui into
"preferences_display", so that modules which just need to query a
value do not end up depending on the video subsystem.  Now that I see
game_status creeping in as a new dependency (and I suspect a real
#include of team.hpp would be needed for the new code on some
platforms) - not quite the same, but definitely too high-level here:
I'd think some of the added stuff would rather belong to somewhere
else (although I'm too tired tonight to try telling where)

- does replay_controller really need that much #includes ?



What is the general opinion on this ?  I'd like to find some way to
make it as easy as possible to go towards a clean dep tree, but don't
want to put more work into this than really needed.  Would it be
sufficient to make sure graph-includes works on all platforms used by
the devs ?  Would it be useful to put an easily browsable version on
the wiki (it is a large file, especially when converted to a bitmap
format !) ?

Probably adding more information at the top of the source files,
telling what they are supposed to be about, would also help.

Any other ideas ?


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.


[1] http://freshmeat.net/projects/graph-includes/
[2] http://ydirson.free.fr/soft/wesnoth/graphs/

-- 
Yann Dirson    <address@hidden> |
Debian-related: <address@hidden> |   Support Debian GNU/Linux:
                                    |  Freedom, Power, Stability, Gratis
     http://ydirson.free.fr/        | Check <http://www.debian.org/>