Sven Neumann wrote:

Hi Martin,

I've found your last commit message quite confusing and so is the FIXME that you added to gimpdisplayconfig.c. I'd very much welcome if such texts would not be added to the code without prior discussion here.

Your comment states:

/* FIXME: If we can get rid of this dependency to the display module, * we will greatly improve the module dependencies in the core, see * devel-docs/gimp-module-dependencies.svg. In particular, we will get * rid of the (transitive) dependency from the core to the UI code */
#include "display/display-enums.h"
#include "display/gimpdisplayoptions.h"

This is quite misleading though, as there is no dependency from the core to the UI code. The two files from app/display that are included here could as well be moved to app/config. If you look at them, you will see that the two files hold nothing but settings. This is config code, it's just very closely related to the display module as it deals with display configuration that needs to be stored in the gimprc. It's just for clarity that these files live in the app/display module. If this leads to confusion, we can certainly discuss to move the files.

If you don't mind, I would like to remove the misleading comment that you added. There is nothing that needs to be fixed here. Except perhaps for the gimp-module-dependencies.svg graphics. I guess I am misunderstanding the purpose of this graphics, but it does not make sense for me and as such it is certainly not helpful for potential contributors. Perhaps we can try to change the graphics so that it helps people who are new to the GIMP code to understand it better?

Hi Sven,

If you find the comment confusing I should explain it in more detail. First, I should define the terms I use in the comment. When I say "UI code" I am referring to any code that is in modules with a GTK+ dependency, which includes the display module. When I say "core", I am referring to any code that is in the core modules, modules that don't have a GTK+ dependency, such as xcf, text, vectors, core and config. With these definitions, which I find reasonable,the core has a dependency to the UI code. If you are fine with moving the two files to the config module, we should do that. Since this is something that IMHO should be fixed, we should not remove the comment.

The purpose of the graphic is to give an overview of how the libraries and core modules in the GIMP code base relate to each other. It gives a nice overview and I believe it will be beneficial both to new and old contributors (I find it useful myself). It can certainly be improved further. I do believe however that we should separate a graphic of the actual architecture (which the graphic represents, it is generated from the GIMP code) from the architecture we want, which should go in a separate file.

Regards,
Martin

Hi again,

Sven Neumann wrote:

Since there is no GTK+ dependency in the two files, nor any dependency on code that has a GTK+ dependency, there is no dependency from core to the UI here. The files are just better located in app/display as that is where they are used.

I don't understand why you insist on reasoning about files when the GIMP code is divided into modules. I agree that if considering only individual files, the core does not have a dependency to the UI code. If we limit our reasoning to modules however, do you then agree with the conclusion that the core has a dependency to UI code?

I would expect to find a direction in the graphics going from base modules to the UI. And I would expect to find modules that are on the same level grouped together. Instead I find chaos. Sorry, but there is no order in this graphics. It's just arrows pointing in all directions. This does certainly not reflect the structure of the current code.

The reason for the mess is that the config module includes files from the display module. If these includes are removed and the graphic regenerated, you end up with this:
http://www.chromecode.com/temp/gimp-module-dependencies.svg

Here you can see that the core modules are dependent on each other but not on the UI modules, and that the UI modules are dependent on each other and the core code. It should also be noted that the dependency graph have been transitively reduced [1], which is why there is no explicit dependency from app/file to app/core even though 'file' depends on 'core'.

Doing further grouping would make the tool that generates the graphic less useful for finding dependencies of the config -> display type.

/ Martin

[1] http://en.wikipedia.org/wiki/Transitive_reduction

Sven Neumann wrote:

When we split up the GIMP core into sub-directories, we also split the code into UI and core. You will find a few files in the UI sub-directories that hold nothing but config settings. Since the non-UI GIMP should be able to read the same config files that the UI-enabled GIMP uses, the non-UI code needs to be know about some config settings that are part of the UI. That's why gimp-console needs to link a few files from the non-UI parts of GIMP. Does that mean that these files should be moved to other folders? No, because they are in the right location, close to the code that actually uses them. Does that mean that there is a dependency that needs to be fixed? No, not unless you want to drop the possibility to share config files between the UI-enabled and the non-UI GIMP.

We have special-casing on the includes in app/config/gimpdisplayconfig.c by including types not in the module namespace, and we have special casing on the linking in gimp-console by including individual files instead of only module libraries. If we were to move the files to the config module, we could remove this special casing. Isn't it obvoius that this is what we should do?

I don't buy the argument that code must be close to the code that uses them. That completely invalidates service layers in an architecture, i.e. layers that provides services to clients in layers above.

And exactly how would moving the display config files to the config module drop the possibility to share config files between the UI-enabled and non-UI GIMP? The UI GIMP has access to the non-UI GIMP.

Best regards, Martin