[Gimp-developer] config - display dependency
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? Sven ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] config - display dependency
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 ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] config - display dependency
Hi, On Tue, 2010-01-05 at 22:46 +0100, Martin Nordholts wrote: 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. 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. 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). 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. Sven ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] config - display dependency
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 ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
