HomePhabricator
Diffusion MITK eac8b99e0769

Added comments and refactored according to review in T24216

Description

Added comments and refactored according to review in T24216

Details

Provenance
kalaliAuthored on Apr 11 2018, 1:31 PM
kalaliPushed on Apr 11 2018, 1:32 PM
Parents
rMITK251e40159685: wip: include recent ideas for display action events and their handling
Branches
Unknown
Tags
Unknown

Event Timeline

/Modules/Core/src/Interactions/mitkStdDisplayActionEventHandler.cpp
58–61

Here we are connecting some default events, that are predefined inside this class.
Is this the part of the code that we might put into an own namespace for everyone to access, as some kind of default action on certain events?

64

An example of such a default action on the move event.
Could be put into an own file / namespace for everyone to access, so that we don't hide it here in this class.

67

We are basically doing the same as in the 'CustomDisplayActionEventHandler'. The difference is that we don't insert the std::function from outside but directly have it here inside the 'Connect'-function.
I think that we might solve this in a more elegant way, e.g. by using a common base class for both the 'StdDisplayActionEventHandler' and the 'CustomDisplayActionEventHandler': This base class could contain the 'ConnectDisplayActionEvent'-function from the 'CustomDisplayActionEventHandler'. Then the 'StdDisplayActionEventHandler'-subclass could access this function and use it to provide an action function and a filter function (which is exactly the function written here).

Basically, we would move the command-initialization, setting it as an observer and pushing the tag to a list to a commonly accessible function (in the base class) and leave the implementation specific creation of the action functions and filter functions to the 'StdDisplayActionEventHandler'.

However, this means, that we don't need a ''CustomDisplayActionEventHandler'' as everything could already be done by this base-class. So should we go back to a "subclass-approach"?

floca added inline comments.
/Modules/Core/src/Interactions/mitkStdDisplayActionEventHandler.cpp
58–61

I think the extraction as a default connect would be basically a good idea. The criteria I see are

  1. Is it likely to be used somewhere else?
  2. Can we cleanly formulate it state less (so without having internals of the handler)

I see no problem with number 2. With number 1 I am not quite sure for all functions. But it would not heard to make them globally reusable for other that want to behave like the StdDisplayActionEventHandler.

If the concrete implementation of the connection should be refactored (e.g. CrosshairEvent does not effect all registered render windows but only a subset (e.g. those of the same stdmultiwidget) is another topic and should be discussed in context (may be best in an explicit reafactoring task when the first draft is done and running.

64

Yes. see above.

67

I think it is a good idea.

/Modules/Core/src/Interactions/mitkStdDisplayActionEventHandler.cpp
58–61

Extracted basic default functions.

67

New commit shows how this could look like.