CR: RED

Issue-Ref: 3815
Signed-off-by: Alexander Diewald <diewald@fortiss.org>

org.fortiss.tooling.base.ui/src/org/fortiss/tooling/base/ui/editor/fx/.ratings

-ContextMenuUtil.java b0eaa896a4a237bad418f788314976a383b1fb8a YELLOW
-EObjectBasedDiagramControllerBase.java 77cad946c06ab73790220c4b07c392f183cc54cb YELLOW
-EObjectBasedRectangularResizableContentControllerBase.java 422211e075f8dc80f5a7096e059822fc39cc81a6 YELLOW
-LayoutedModelElementBasedContentAnchorageController.java 37e91dc8639966cb7f802182211105f61385072d YELLOW
-LayoutedModelElementBasedDiagramAnchorageController.java a20392301d2cbb2cd85dfc391090453a2cb3b674 YELLOW
-LayoutedModelElementBasedLinkBendPointController.java 4b4a5b81186fd485973f2a407648c50f7115ca57 YELLOW
+ContextMenuUtil.java 7f72dbd78a3dd9da2c083dd325ba7a686c7be778 RED
+EObjectBasedDiagramControllerBase.java 66eb20fa8f6e320009226e1c284531d0be138367 RED
+EObjectBasedRectangularResizableContentControllerBase.java 79f3006141306ca3173c4f5f20121b46ce3fd009 RED
+LayoutedModelElementBasedContentAnchorageController.java 95535008b6468e459c70d8a987e021fb0c17f4b8 RED
+LayoutedModelElementBasedDiagramAnchorageController.java 785aea3cdd5501ebf60f3096b54192beb5394086 RED
+LayoutedModelElementBasedLinkBendPointController.java 8ff042ebd226e6e65aa860f0845ceddc4c117610 RED

org.fortiss.tooling.base.ui/src/org/fortiss/tooling/base/ui/editor/fx/ContextMenuUtil.java

@@ -43,6 +43,7 @@ final class ContextMenuUtil {
 	/** Creates the menu populated with composable prototypes. */
 	public static List<MenuItem> createPrototypeMenu(EObject target,
 			IElementCompositionContext context) {
+		// TODO(AD): Maybe get the used services in the beginning for more compact functional code.
 		List<MenuItem> result = new ArrayList<>();
 		Menu newMenu = new Menu("New ...");
 		List<Prototype> protos =
@@ -52,6 +53,7 @@ final class ContextMenuUtil {
 			EObject prototypeCopy = p.getPrototypeCopy();
 			if(ecs.canCompose(target, prototypeCopy, context)) {
 				MenuItem mi = new MenuItem(p.getName());
+				// TODO(AD): Avoid curly braces in lambdas for single statements.
 				mi.setOnAction(evt -> {
 					ICommandStackService.getInstance().runAsCommand(target, () -> {
 						ecs.compose(target, prototypeCopy, context);
@@ -80,12 +82,14 @@ final class ContextMenuUtil {
 		// link highlighting
 		if(features.isLinkHighlightingEnabled()) {
 			MenuItem disableLinkHighlighting = new MenuItem("Disable Link Highlighting");
+			// TODO(AD): Avoid curly braces in lambdas for single statements.
 			disableLinkHighlighting.setOnAction(evt -> {
 				features.setLinkHighlightingEnabled(false);
 			});
 			menu.getItems().add(disableLinkHighlighting);
 		} else {
 			MenuItem enableLinkHighlighting = new MenuItem("Enable Link Highlighting");
+			// TODO(AD): Avoid curly braces in lambdas for single statements.
 			enableLinkHighlighting.setOnAction(evt -> {
 				features.setLinkHighlightingEnabled(true);
 			});
@@ -94,12 +98,14 @@ final class ContextMenuUtil {
 		// interaction area shading
 		if(features.isInteractionAreaShadingEnabled()) {
 			MenuItem disableInteractionShading = new MenuItem("Disable Interactive Area Shading");
+			// TODO(AD): Avoid curly braces in lambdas for single statements.
 			disableInteractionShading.setOnAction(evt -> {
 				features.setInteractionAreaShadingEnabled(false);
 			});
 			menu.getItems().add(disableInteractionShading);
 		} else {
 			MenuItem enableInteractionShading = new MenuItem("Enable Interactive Area Shading");
+			// TODO(AD): Avoid curly braces in lambdas for single statements.
 			enableInteractionShading.setOnAction(evt -> {
 				features.setInteractionAreaShadingEnabled(true);
 			});

org.fortiss.tooling.base.ui/src/org/fortiss/tooling/base/ui/editor/fx/EObjectBasedDiagramControllerBase.java

@@ -47,7 +47,12 @@ import javafx.scene.control.MenuItem;
 /**
  * Diagram controller that uses the {@link IElementCompositorService} to create context menu
  * entries for prototypes provided by the {@link IContextMenuService}.
+ * 
+ * @author hoelzl
  */
+// TODO(AD): There are several duplicates in the implemented methods across the controllers in this
+// package: Please consider adding another base class. For instance, the addListener/removeListener
+// methods could be unified.
 public abstract class EObjectBasedDiagramControllerBase extends ControllerBase
 		implements IModelChangeProvider {
 	/** Notification adapter for model change events. */
@@ -77,6 +82,11 @@ public abstract class EObjectBasedDiagramControllerBase extends ControllerBase
 		double y = max(features.getVerticalSpacing(), diagramLocation.getY());
 
 		// wild cast works: see constructor exception
+		// TODO(AD):
+		// * Are there use cases where the IContentMVCBundle is not an EObject?
+		// * Is tooling.common required to support any kind of model?
+		// * Would it be a viable alternative to parameterize the IContentMVCBundle class with the
+		// model type?
 		EObject modelParent = (EObject)getModel();
 		IElementCompositionContext edc = createElementCompositionContext(modelParent, x, y, true,
 				getViewer().getFeatures().getCurrentZoomFactor());
@@ -93,6 +103,7 @@ public abstract class EObjectBasedDiagramControllerBase extends ControllerBase
 	@Override
 	public void addListener(IModelChangeListener l) {
 		if(changeListeners.isEmpty()) {
+			// Lazily install model listeners when the first listener is installed here.
 			connectToModel();
 		}
 		if(!changeListeners.contains(l)) {

org.fortiss.tooling.base.ui/src/org/fortiss/tooling/base/ui/editor/fx/EObjectBasedRectangularResizableContentControllerBase.java

@@ -38,7 +38,12 @@ import javafx.scene.control.MenuItem;
 /**
  * {@link RectangularResizableContentControllerBase} with a listener mechanism connected to
  * {@link EObject#eAdapters()}.
+ * 
+ * @author hoelzl
  */
+// TODO(AD): There are several duplicates in the implemented methods across the controllers in this
+// package: Please consider adding another base class. For instance, the addListener/removeListener
+// methods could be unified.
 public abstract class EObjectBasedRectangularResizableContentControllerBase
 		extends RectangularResizableContentControllerBase implements IModelChangeProvider {
 	/** Layout model adapter used for listening to EMF model changes. */

org.fortiss.tooling.base.ui/src/org/fortiss/tooling/base/ui/editor/fx/LayoutedModelElementBasedContentAnchorageController.java

@@ -36,7 +36,12 @@ import org.fortiss.tooling.kernel.service.IElementCompositorService;
 /**
  * {@link DelegatingContentAnchorageController} for
  * {@link org.fortiss.tooling.base.model.layout.ILayoutedModelElement}s.
+ * 
+ * @author hoelzl
  */
+// TODO(AD): There are several duplicates in the implemented methods across the controllers in this
+// package: Please consider adding another base class. For instance, the addListener/removeListener
+// methods could be unified.
 public class LayoutedModelElementBasedContentAnchorageController
 		extends DelegatingContentAnchorageController implements IModelChangeProvider {
 	/** Layout model adapter used for listening to EMF model changes. */

org.fortiss.tooling.base.ui/src/org/fortiss/tooling/base/ui/editor/fx/LayoutedModelElementBasedDiagramAnchorageController.java

@@ -34,7 +34,14 @@ import org.fortiss.tooling.base.utils.LayoutModelElementAdapter;
 import org.fortiss.tooling.kernel.service.IConnectionCompositorService;
 import org.fortiss.tooling.kernel.service.IElementCompositorService;
 
-/** {@link MoveControllerBase} for {@link ILayoutedModelElement}s. */
+/**
+ * {@link MoveControllerBase} for {@link ILayoutedModelElement}s.
+ * 
+ * @author hoelzl
+ */
+// TODO(AD): There are several duplicates in the implemented methods across the controllers in this
+// package: Please consider adding another base class. For instance, the addListener/removeListener
+// methods could be unified.
 public class LayoutedModelElementBasedDiagramAnchorageController extends MoveControllerBase
 		implements IModelChangeProvider {
 	/** Layout model adapter used for listening to EMF model changes. */

org.fortiss.tooling.base.ui/src/org/fortiss/tooling/base/ui/editor/fx/LayoutedModelElementBasedLinkBendPointController.java

@@ -38,6 +38,8 @@ import org.fortiss.tooling.kernel.service.IConnectionCompositorService;
 /**
  * {@link IController} for {@link ILayoutedModelElement}s representing links and connections in a
  * {@link DiagramViewer}.
+ * 
+ * @author hoelzl
  */
 public class LayoutedModelElementBasedLinkBendPointController extends LinkControllerBase
 		implements IModelChangeProvider {

org.fortiss.tooling.base/src/org/fortiss/tooling/base/utils/.ratings

@@ -6,7 +6,7 @@ ConstraintsBaseUtils.java bba938b43756ce6f35c338f6cef21c3ab5d49a9d GREEN
 DimensionUtils.java 0dc67f9de11a84e6e4c6e1eb627817dee91ff30a GREEN
 EllipseLayoutUtils.java 5c3a0126fdca5d5b4fb441694747e1cb0f49cd9f GREEN
 LayoutDataUtils.java 5739dd16f0212e8f94927c0a0f51503390f2be69 GREEN
-LayoutModelElementAdapter.java 1a1c9700513ec2e74f99ea20a830bf8d0db5983a YELLOW
+LayoutModelElementAdapter.java fcbd25ff4fa93e10692cdbbe7a2adf6cd04b93a2 RED
 LayoutModelElementFactory.java c49fca2de5a8cb7502fb28cc2b7e64a272df11b0 GREEN
 MigrationUtils.java ab9d8682233313c21c6a52b8b03e1c796aacd29c GREEN
 OffsetOrientationUtils.java 913cebbac2a5628bdd08b4df669b9412d6e07d94 GREEN

org.fortiss.tooling.base/src/org/fortiss/tooling/base/utils/LayoutModelElementAdapter.java

@@ -48,6 +48,8 @@ public class LayoutModelElementAdapter extends EContentAdapter implements Comman
 	private final List<Notification> notifications = new ArrayList<>();
 
 	/** Constructor. */
+	// TODO(AD): The service references could also be moved to fields to save some chars/method
+	// accesses.
 	public LayoutModelElementAdapter(ILayoutedModelElement layouted) {
 		this.modelElement = requireNonNull(layouted);
 		modelElement.eAdapters().add(this);
@@ -71,6 +73,8 @@ public class LayoutModelElementAdapter extends EContentAdapter implements Comman
 		if(notifier == modelElement) {
 			Object feature = notification.getFeature();
 			if(feature instanceof EAttribute) {
+				// TODO(AD): Just for my understanding: Why is it required to add the notification
+				// in this case?
 				notifications.add(notification);
 				// no selfAdapt needed for attributes
 				return;

org.fortiss.tooling.kernel.ui/src/org/fortiss/tooling/kernel/ui/extension/base/.ratings

@@ -7,7 +7,7 @@ EReferencePropertySectionBase.java 0548da6778516003257f59d0b4c2b60d458be3b6 GREE
 EditorBase.java 9c09fff92945256bb8680992ae7bb2c78f47b150 GREEN
 FXEditorBase.java 2e520be0bbae7d0aebdff70218a124dbe0896ce2 YELLOW
 IListPropertySection.java 8bb00fe7959583e794ff9437b7a77404c9a9e70f GREEN
-LWFXEFEditorBase.java ab11a24ea8d5ef9fe6d37eca2e11092c40791dc5 YELLOW
+LWFXEFEditorBase.java 5c53864d395ec15a0b31fa4e33cc5384d01d6f0a RED
 ModelEditorBindingBase.java 4c5ac569c0b6e7678fc8191096b26dfd09fdcb98 GREEN
 ModelElementHandlerBase.java 384727748f125c9d43f19d9c0eba4ba1be5a7a26 GREEN
 MultiEObjectActionBase.java 9e237d8ea640c4194e4877af4a9cfce88698e543 GREEN

org.fortiss.tooling.kernel.ui/src/org/fortiss/tooling/kernel/ui/extension/base/LWFXEFEditorBase.java

@@ -50,6 +50,7 @@ public abstract class LWFXEFEditorBase<T extends EObject> extends FXEditorBase<T
 
 	/** {@inheritDoc} */
 	@Override
+	// TODO(AD): Avoid curly braces in lambdas for single statements.
 	protected final Parent createSceneRoot() {
 		viewer = new DiagramViewer(createModelFactory(), createVisualFactory(),
 				createControllerFactory(), cb -> {
@@ -83,6 +84,7 @@ public abstract class LWFXEFEditorBase<T extends EObject> extends FXEditorBase<T
 
 	/** Called when some change to the model happens. */
 	protected void applyModelChange(Change chg) {
+		// TODO(AD): Avoid curly braces in lambdas for single statements.
 		ICommandStackService.getInstance().runAsCommand(editedObject, () -> {
 			chg.applyChange();
 		});
@@ -118,6 +120,7 @@ public abstract class LWFXEFEditorBase<T extends EObject> extends FXEditorBase<T
 	}
 
 	/** Creates an {@link IStructuredSelection} from {@link DiagramViewerSelection}. */
+	// TODO(AD): Can't this be put directly in the getSelection method?
 	private IStructuredSelection createSelection() {
 		DiagramViewerSelection vsel = viewer.getSelection();
 		if(vsel.isEmpty()) {