Major bug!!!!

[minovations]

Tree pointers and Structure pointers are when using the configuration macros. By changing the Tree pointers to void pointers, the system was compilable, but the fundamental way of working was killed.

I will pick this issue up and try to fix it

[minovations]

Why is libconfiguration necessary? I understand that it is easy to have one point where all active variables are stored. But it is not smart to made it Independent of lib-common, because the configuration struct is heavenly depending on internal types.

I plea for moving the configuration struct to the lib-common library.

[roelj]

Tree pointers and Structure pointers are when using the configuration macros. By changing the Tree pointers to void pointers, the system was compilable, but the fundamental way of working was killed.

What exactly is the problem?

Why is libconfiguration necessary?

To have one point where all active variables are stored.

But it is not smart to made it Independent of lib-common, because the configuration struct is heavenly depending on internal types.

It doesn't directly because it doesn't need to know anything about the type. It only stores and tells pointers.

It doesn't fit into libcommon because it is not generically applicable to other programs. (For example, we've split up libcommon/tree and libmemory/tree to separate the generically applicable part from the program-specific part.)

The point of libconfiguration is that it provides access to settings to all other libraries. It has no other dependencies, so it's a very lightweight library that can be included by any other part without causing dependency resolving trouble.

Moving it into libcommon is possible, I guess. The only gain here is that you can change the void pointers to the specific type, which is not needed because libconfiguration shouldn't be doing anything other than storing and telling the location of a variable.

If you insist on libconfiguration needing to know exactly what type a variable is, you could also make libconfiguration dependent on the datatypes provided by libcommon, which would effectively achieve the same result.

[minovations]

What exactly is the problem?

I found out that the active_serie as active_mask is filled with tree and struct pointers. The compiller accepts any pointer, so does not warn you if parsing a wrong type of pointer. We could debat over which fucntionality we should an would use of the compiler, dut for my that is useless. There are types defined, so use them.

It doesn't directly because it doesn't need to know anything about the type. It only stores and tells pointers.

Abstractly looking to that piece of code, you are wright it only sets and gets pointers. But again, libconfiguration on its own is nothing, you will not use it in other software (which is not related to clmedview). So there is an indirect relation between libconfiguration and clmedview. Therefore i plea for integrating the types.

The point of libconfiguration is that it provides access to settings to all other libraries. It has no other dependencies, so it's a very lightweight library that can be included by any other part without causing dependency resolving trouble.

I understand the use of one common place where all active pointers are stored, that is a feature which is needed. Nevertheless, I would prefer that the types are included.

If you insist on libconfiguration needing to know exactly what type a variable is, you could also make libconfiguration dependent on the data types provided by libcommon, which would effectively achieve the same result.

I think that this is the best way of clarifying this piece of software.

My proposal to make it dependent of libcommon-tree, and libcommon-list and skip the active_serie_struct

typedef struct
{
  Tree *pt_memory;            /*< The actively maintained memory tree. */
  Tree *pt_active_study;      /*< The active study element in the tree. */
  Tree *pt_active_serie;      /*< The active study element in the tree. */
  Tree *pt_active_mask;         /*< The active study element in the tree. */

  List *pl_draw_tools;        /*< A list of all loaded draw tools. */
  List *pl_lookup_tables;     /*< A list of lookup tables. */
  List *pl_active_draw_tool;  /*< The active draw tool element in the list. */

  char c_key_bindings[14];    /*< An array with key bindings. */
} Configuration;

[roelj]

I found out that the active_serie as active_mask is filled with tree and struct pointers. The compiller accepts any pointer, so does not warn you if parsing a wrong type of pointer. We could debat over which fucntionality we should an would use of the compiler, dut for my that is useless. There are types defined, so use them.

Ok.

Abstractly looking to that piece of code, you are wright it only sets and gets pointers. But again, libconfiguration on its own is nothing, you will not use it in other software (which is not related to clmedview). So there is an indirect relation between libconfiguration and clmedview. Therefore i plea for integrating the types.

The reason it has been separated from libcommon is that it is not reusable in other software. Yes, libconfiguration is bound to clmedview, just like any other library other than those in libcommon.

Integrating the types of libcommon is a separate issue. As long as libconfiguration is not moved into libcommon for the reasons stated above, I'm fine with it.

I understand the use of one common place where all active pointers are stored, that is a feature which is needed. Nevertheless, I would prefer that the types are included.

Ok.

I think that this is the best way of clarifying this piece of software.

My proposal to make it dependent of libcommon-tree, and libcommon-list and skip the active_serie_struct

typedef struct
{
  Tree *pt_memory;            /*< The actively maintained memory tree. */
  Tree *pt_active_study;      /*< The active study element in the tree. */
  Tree *pt_active_serie;      /*< The active study element in the tree. */

  List *pl_draw_tools;        /*< A list of all loaded draw tools. */
  List *pl_lookup_tables;     /*< A list of lookup tables. */
  List *pl_active_draw_tool;  /*< The active draw tool element in the list. */

  char c_key_bindings[14];    /*< An array with key bindings. */
} Configuration;

Ok. That looks good. It allows the compiler to detect wrong behavior. Could you implement the change?

[minovations]

I am working on the changes

[minovations]

I think that it is confusing where all active pointers are located. E.g. 'ps_active_viewer' is located in main_gui. 'pl_active_draw_tool' is located in libconfiguration. I moved 'pl_active_draw_tool' to main gui, but is this correct?

Why is 'pl_draw_tools' in configuration, this is a thing of pixeldata? Why is 'pl_lookup_tables' in configuration, this is a thing of pixeldata?

I think we should visualize what should be where and correct if necessary.

[roelj]

I think that we should put everything that can be considered "the program's state" into libconfiguration. So when the user clicks somewhere that changes the active mask layer, that must be recorded somewhere (as the program's state).

So I'd say pl_active_draw_tool should stay in libconfiguration.

The GUI component creates three Viewer instances (at all times, when a file is loaded). There's nothing a user can do about it, and it can therefore be considered as a part of GUI instead of the program's state.

I think the active viewer pointer should move into libconfiguration, because it's program state.

[roelj]

I see the merged code now contains Tree pointers instead of direct Serie and Study pointers. I really think this is a step backwards in terms of responsibility within the code.

So I guess whatever got pushed is the way forward for radmedres:clmedview. So there's no need to keep this issue open.

[minovations]

Roel,

Thanks for your opinion. I'm sorry that your thinking this is a step back. As proposed I will close this issue.