Update Review (1 RED). Note done yet.

Issue-Ref: 4310
Issue-Url: af3#4310



Signed-off-by: Simon Barner <barner@fortiss.org>

org.fortiss.tooling.ext.quality.ui/META-INF/MANIFEST.MF

@@ -5,14 +5,9 @@ Bundle-SymbolicName: org.fortiss.tooling.ext.quality.ui;singleton:=true
 Bundle-Version: 2.23.0.qualifier
 Automatic-Module-Name: org.fortiss.tooling.ext.quality.ui
 Bundle-RequiredExecutionEnvironment: JavaSE-11
-Require-Bundle: org.eclipse.ui.ide;visibility:=reexport,
- org.fortiss.tooling.base.ui;visibility:=reexport,
- org.fortiss.tooling.kernel,
- org.fortiss.tooling.kernel.ui,
- org.fortiss.tooling.ext.quality,
- org.fortiss.tooling.spiderchart.ui,
- org.eclipse.emf.ecore,
- org.fortiss.af3.project;bundle-version="2.23.0"
+Require-Bundle: org.fortiss.tooling.base.ui;visibility:=reexport,
+ org.fortiss.tooling.ext.quality;visibility:=reexport,
+ org.fortiss.tooling.spiderchart.ui;visibility:=reexport
 Export-Package: org.fortiss.tooling.ext.quality.ui.view.fx
 Bundle-Activator: org.fortiss.tooling.ext.quality.ui.ModelQualityUIActivator
 Bundle-ActivationPolicy: lazy

org.fortiss.tooling.ext.quality.ui/src/org/fortiss/tooling/ext/quality/ui/.ratings

-ModelQualityExtractionMenu.java 3bdbf3d37715539648af8ba7feefbec9370c4dc4 YELLOW
+ModelQualityExtractionMenu.java d9282c7df44aa9461ec662beb557e74589f6b05f GREEN
 ModelQualityUIActivator.java 2f47db82bc088072dbfe53f82d3ed417d77fd23c GREEN

org.fortiss.tooling.ext.quality.ui/src/org/fortiss/tooling/ext/quality/ui/ModelQualityExtractionMenu.java

@@ -47,9 +47,12 @@ public class ModelQualityExtractionMenu implements IContextMenuContributor {
 	@Override
 	public List<IContributionItem> getContributedItems(EObject selection,
 			ContextMenuContextProvider contextProvider) {
+
 		if(!(selection.eContainer() != null)) {
 			return emptyList();
 		}
+
+		// A null eContainer indicates the model root
 		return asList(new IContributionItem[] {
 				new ActionContributionItem(new MetricExtractionAction(selection))});
 	}
@@ -68,19 +71,20 @@ public class ModelQualityExtractionMenu implements IContextMenuContributor {
 	/** Action for generating the set of . */
 	protected class MetricExtractionAction extends Action {
 
-		/** {@link FileProject} on which the action will be triggered. */
-		private final EObject fp;
+		/** Model on which the action will be triggered. */
+		private final EObject model;
 
 		/** Constructor. */
-		public MetricExtractionAction(EObject fp) {
+		public MetricExtractionAction(EObject model) {
 			super(MENU_NAME, getActionIcon());
-			this.fp = fp;
+			this.model = model;
 		}
 
 		/** {@inheritDoc} */
 		@Override
 		public void run() {
-			ITopLevelElement top = IPersistencyService.getInstance().getTopLevelElementFor(this.fp);
+			ITopLevelElement top =
+					IPersistencyService.getInstance().getTopLevelElementFor(this.model);
 			IModelQualityService.getInstance().scheduleMetricCollection(top);
 		}
 	}

org.fortiss.tooling.ext.quality.ui/src/org/fortiss/tooling/ext/quality/ui/view/fx/.ratings

 IModelQualityViewPart.java 708f8089645df12098ea67190805cce343045d2e GREEN
-ModelQualityFXController.java 67bb0a067c578ac9a9b30e5973587cc57374a782 YELLOW
-ModelQualityFXViewPart.java a1e82fe68a3fe7d813fa61d5148ffcd1592000f7 YELLOW
+ModelQualityFXController.java 64c5fb2ca8cca77982b5a20695efcf2b1442045e RED
+ModelQualityFXViewPart.java 9cfc7f60a86ea5b915c726b16712f8be7dec2c5f GREEN

org.fortiss.tooling.ext.quality.ui/src/org/fortiss/tooling/ext/quality/ui/view/fx/ModelQualityFXController.java

@@ -15,10 +15,10 @@
 +--------------------------------------------------------------------------*/
 package org.fortiss.tooling.ext.quality.ui.view.fx;
 
+import static java.util.stream.Collectors.toList;
 import static javafx.scene.paint.Color.BLUE;
 import static javafx.scene.paint.Color.DARKGRAY;
 import static javafx.scene.paint.Color.LIGHTGRAY;
-import static java.util.stream.Collectors.toList;
 import static org.fortiss.tooling.common.ui.javafx.style.FontStyle.BLACK_VERDANA_10PT;
 import static org.fortiss.tooling.common.ui.javafx.style.FontStyle.BLACK_VERDANA_12PT;
 import static org.fortiss.tooling.common.ui.javafx.style.FontStyle.BLACK_VERDANA_14PT;
@@ -27,7 +27,6 @@ import static org.fortiss.tooling.common.ui.javafx.style.LineStyle.SOLID_BLACK_1
 import static org.fortiss.tooling.kernel.ui.util.SelectionUtils.checkAndPickFirst;
 import static org.fortiss.tooling.kernel.utils.EcoreUtils.getFirstParentWithType;
 
-
 import java.text.DecimalFormat;
 import java.time.format.DateTimeFormatter;
 import java.util.ArrayList;
@@ -35,7 +34,6 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 
-
 import org.eclipse.emf.ecore.EObject;
 import org.eclipse.jface.viewers.ISelection;
 import org.eclipse.ui.ISelectionListener;
@@ -153,8 +151,6 @@ public class ModelQualityFXController extends CompositeFXControllerBase<SplitPan
 		// Invalidate last selection, so if no new selection is found the charts are cleared
 		lastSelectedElement = null;
 
-		IModelQualityService mqs = IModelQualityService.getInstance();
-
 		EObject selected = checkAndPickFirst(selection, EObject.class);
 
 		if(selected == null) {
@@ -170,6 +166,8 @@ public class ModelQualityFXController extends CompositeFXControllerBase<SplitPan
 			updateCharts("Selection does not have a project root element.");
 			return;
 		}
+
+		IModelQualityService mqs = IModelQualityService.getInstance();
 		var rootDataElement = mqs.getMetricData().getDataRootElementMap().get(currentRootElement);
 
 		if(rootDataElement == null) {
@@ -209,14 +207,10 @@ public class ModelQualityFXController extends CompositeFXControllerBase<SplitPan
 	private void updateCharts(String message) {
 
 		if(lastSelectedElement != null) {
-			MetricData metricData =
-					IModelQualityService.getInstance().getMetricData();
+			MetricData metricData = IModelQualityService.getInstance().getMetricData();
 			MetricTreeNode node = metricData.getTreeNodeLookupTable().get(lastSelectedElement);
 
 			if(node != null) {
-
-				// updateChoiceBoxes(node);
-
 				if(!node.getChildren().isEmpty()) {
 
 					updatePieChart(node);
@@ -334,18 +328,21 @@ public class ModelQualityFXController extends CompositeFXControllerBase<SplitPan
 	private SpiderChartViewer buildSpiderChart() {
 		// Calculate the intersection of the keys, i.e. a set of keys which have a non-null value in
 		// all elements of the lastViewedElements list
+		// TODO (SB): Add check that lastViewedElements is non empty, and add a corresonding comment
+		// to the .get(0) statement
 		Set<MetricKey> keysIntersection = lastViewedElements.get(0).getValue();
 		for(var p : lastViewedElements) {
 			keysIntersection.retainAll(p.getValue());
 		}
 
 		// Create DataSeries for all elements
-		List<Pair<MetricTreeNode, DataSeries>> dataSeriesList = lastViewedElements.stream()
-				.map(pair -> new Pair<>(pair.getKey(), 
-					(pair.getKey().getName() == null) ? new DataSeries("No Name found") : new DataSeries(pair.getKey().getName())
-				))
-				.collect(toList());
-
+		List<Pair<MetricTreeNode, DataSeries>> dataSeriesList =
+				lastViewedElements.stream().map(pair -> {
+					MetricTreeNode key = pair.getKey();
+					String name = key.getName();
+					return new Pair<>(key, (name == null) ? new DataSeries("No name found")
+							: new DataSeries(name));
+				}).collect(toList());
 
 		// Create SpiderChart and set basic styling
 		SpiderChart spiderChart = new SpiderChart();
@@ -384,6 +381,8 @@ public class ModelQualityFXController extends CompositeFXControllerBase<SplitPan
 		for(var pair : dataSeriesList) {
 			spiderChart.addData(pair.getValue());
 			// Assign different colors to different data series
+			// updateLastViewedElements() ensures that the are are at most three elements in
+			// updateLastViewedElements, from which dataSeriesList is derived above.
 			Color color = SPIDER_CHART_OVERLAY_COLORS[i];
 			LineStyle olive1pt = new LineStyle(color);
 			FillStyle oliveFill = new FillStyle(color, 0.25);
@@ -415,10 +414,8 @@ public class ModelQualityFXController extends CompositeFXControllerBase<SplitPan
 				return;
 			}
 
-			// Store IModelQualityService.getInstance() (sic!) in a variable (e.g. "mqs") for
-			// readability
-			var rootDataElement = IModelQualityService.getInstance().getMetricData()
-					.getDataRootElementMap().get(updatedElement);
+			IModelQualityService mqs = IModelQualityService.getInstance();
+			var rootDataElement = mqs.getMetricData().getDataRootElementMap().get(updatedElement);
 			if(rootDataElement != null) {
 				bottomText.setText("Metrics last updated: " +
 						timeFormat.format(rootDataElement.getLastRefresh()) +

org.fortiss.tooling.ext.quality.ui/src/org/fortiss/tooling/ext/quality/ui/view/fx/ModelQualityFXViewPart.java

@@ -27,9 +27,6 @@ import javafx.scene.Scene;
  */
 public class ModelQualityFXViewPart extends AF3FXViewPart implements IModelQualityViewPart {
 
-	// TODO (SB): Is there a reason to make the controller static? (also rename to viewController)
-	// FIXME (KB): Naming convention and static taken from reuse library. So you want me to change the FX controller class to viewController?
-	// Would result in inconsistent use otherwise. still change it and add the view controller as parameter for instancing a View Part?
 	/** The FX Controller for this view. */
 	private static final ModelQualityFXController VIEW_CONTROLLER = new ModelQualityFXController();
 
@@ -52,7 +49,6 @@ public class ModelQualityFXViewPart extends AF3FXViewPart implements IModelQuali
 	@Override
 	protected Scene createFxScene() {
 		Scene scene = super.createFxScene();
-
 		getSite().getWorkbenchWindow().getSelectionService().addSelectionListener(VIEW_CONTROLLER);
 
 		return scene;