changeset 93:a5af2548522b

JMC-6241: Leaking Context Menu Items Summary: Fix the leaking of thread activity context menu items Reviewd by: hirt, jmatsuoka Contributed-by: Ken Dobson <kdobson@redhat.com>
author jmatsuoka <jmatsuok@redhat.com>
date Mon, 17 Dec 2018 16:51:30 -0500
parents 2abad98fa3fc
children c4b6a87219c3
files application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/ThreadGraphLanes.java application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/GarbageCollectionsPage.java application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/JavaApplicationPage.java application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/ThreadsPage.java
diffstat 4 files changed, 40 insertions(+), 22 deletions(-) [+]
line wrap: on
line diff
--- a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/ThreadGraphLanes.java	Thu Dec 13 16:47:07 2018 +0100
+++ b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/ThreadGraphLanes.java	Mon Dec 17 16:51:30 2018 -0500
@@ -99,14 +99,14 @@
 		this.actions = new ArrayList<>();
 	}
 
-	public void openEditLanesDialog(MCContextMenuManager mm) {
+	public void openEditLanesDialog(MCContextMenuManager mm, boolean isLegendMenu) {
 		// FIXME: Might there be other interesting events that don't really have duration?
 		EventTypeFolderNode typeTree = dataSourceSupplier.get().getTypeTree(ItemCollectionToolkit
 				.stream(dataSourceSupplier.get().getItems()).filter(this::typeWithThreadAndDuration));
 		laneDefs = LaneEditor.openDialog(typeTree, laneDefs.stream().collect(Collectors.toList()),
 				Messages.JavaApplicationPage_EDIT_THREAD_LANES_DIALOG_TITLE,
 				Messages.JavaApplicationPage_EDIT_THREAD_LANES_DIALOG_MESSAGE);
-		updateContextMenu(mm);
+		updateContextMenu(mm, isLegendMenu);
 		buildChart.run();
 	}
 
@@ -214,16 +214,26 @@
 		naLanes = lanesByApplicability.get(false);
 		return Collections.emptyList();
 	}
+	
+	//create two action identifiers to handle the chart context menu and the legend context menu
+	private List<String> chartActionIdentifiers = new ArrayList<>();
+	private List<String> legendActionIdentifiers = new ArrayList<>();
 
-	private List<String> actionIdentifiers = new ArrayList<>();
-
-	public void updateContextMenu(MCContextMenuManager mm) {
-		for (String id : actionIdentifiers) {
-			mm.remove(id);
+	public void updateContextMenu(MCContextMenuManager mm, boolean isLegendMenu) {
+		
+		if(isLegendMenu) {
+			for (String id : legendActionIdentifiers) {
+				mm.remove(id);
+			}
+			legendActionIdentifiers.clear();
+		} else { 	
+			for (String id : chartActionIdentifiers) {
+				mm.remove(id);
+			}
+			chartActionIdentifiers.clear();
 		}
-		actionIdentifiers.clear();
 		if (mm.indexOf(EDIT_LANES) == -1) {
-			IAction action = ActionToolkit.action(() -> this.openEditLanesDialog(mm),
+			IAction action = ActionToolkit.action(() -> this.openEditLanesDialog(mm, isLegendMenu),
 					Messages.JavaApplicationPage_EDIT_THREAD_LANES_ACTION,
 					FlightRecorderUI.getDefault().getMCImageDescriptor(ImageConstants.ICON_LANES_EDIT));
 			action.setId(EDIT_LANES);
@@ -245,7 +255,11 @@
 			};
 			String identifier = ld.getName() + checkAction.hashCode();
 			checkAction.setId(identifier);
-			actionIdentifiers.add(identifier);
+			if(isLegendMenu) {
+				legendActionIdentifiers.add(identifier);
+			} else {
+				chartActionIdentifiers.add(identifier);
+			}
 			checkAction.setChecked(ld.isEnabled());
 			// FIXME: Add a tooltip here
 			mm.add(checkAction);
--- a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/GarbageCollectionsPage.java	Thu Dec 13 16:47:07 2018 +0100
+++ b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/GarbageCollectionsPage.java	Mon Dec 17 16:51:30 2018 -0500
@@ -54,6 +54,7 @@
 import org.eclipse.jface.action.IAction;
 import org.eclipse.jface.resource.ImageDescriptor;
 import org.eclipse.jface.viewers.ArrayContentProvider;
+import org.eclipse.jface.viewers.CheckboxTableViewer;
 import org.eclipse.jface.viewers.ColumnViewerToolTipSupport;
 import org.eclipse.jface.viewers.IStructuredSelection;
 import org.eclipse.jface.viewers.TableViewer;
@@ -335,8 +336,10 @@
 
 			gcInfoFolder = new CTabFolder(tableSash, SWT.NONE);
 			phasesList = PHASES.buildWithoutBorder(gcInfoFolder, TableSettings.forState(state.getChild(PHASE_LIST)));
-			phasesList.getManager().getViewer().addSelectionChangedListener(
-					e -> pageContainer.showSelection(ItemCollectionToolkit.build(phasesList.getSelection().get())));
+			phasesList.getManager().getViewer().addSelectionChangedListener(e -> {
+					buildChart();	
+					pageContainer.showSelection(ItemCollectionToolkit.build(phasesList.getSelection().get()));
+			});
 			phasesFilter = FilterComponent.createFilterComponent(phasesList, phasesFilterState,
 					getDataSource().getItems().apply(JdkFilters.GC_PAUSE_PHASE),
 					pageContainer.getSelectionStore()::getSelections, this::onPhasesFilterChange);
@@ -372,14 +375,14 @@
 			chartCanvas = new ChartCanvas(chartContainer);
 			chartCanvas.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true));
 			ActionToolkit.loadCheckState(state.getChild(CHART), allChartSeriesActions.stream());
-			Control chartLegend = ActionUiToolkit.buildCheckboxControl(chartContainer,
-					allChartSeriesActions.stream().filter(a -> includeAttribute(a.getId())), true);
+			CheckboxTableViewer chartLegend = ActionUiToolkit.buildCheckboxViewer(chartContainer,
+					allChartSeriesActions.stream().filter(a -> includeAttribute(a.getId())));
 			GridData gd = new GridData(SWT.FILL, SWT.FILL, false, true);
 			gd.widthHint = 180;
-			chartLegend.setLayoutData(gd);
+			chartLegend.getControl().setLayoutData(gd);
 			lanes = new ThreadGraphLanes(() -> getDataSource(), () -> buildChart());
 			lanes.initializeChartConfiguration(Stream.of(state.getChildren(THREAD_LANES)));
-			IAction editLanesAction = ActionToolkit.action(() -> lanes.openEditLanesDialog(mm),
+			IAction editLanesAction = ActionToolkit.action(() -> lanes.openEditLanesDialog(mm, false),
 					Messages.ThreadsPage_EDIT_LANES, FlightRecorderUI.getDefault().getMCImageDescriptor(ImageConstants.ICON_LANES_EDIT));
 			form.getToolBarManager().add(editLanesAction);
 			
@@ -404,7 +407,8 @@
 			phasesList.getManager().setSelectionState(phasesSelection);
 			metaspaceList.getManager().setSelectionState(metaspaceSelection);
 			mm = (MCContextMenuManager) chartCanvas.getContextMenu();
-			lanes.updateContextMenu(mm);
+			lanes.updateContextMenu(mm, false);
+			lanes.updateContextMenu(MCContextMenuManager.create(chartLegend.getControl()), true);
 			
 			// Older recordings may not have thread information in pause events.
 			// In those cases there is no need for the thread activity actions.
--- a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/JavaApplicationPage.java	Thu Dec 13 16:47:07 2018 +0100
+++ b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/JavaApplicationPage.java	Mon Dec 17 16:51:30 2018 -0500
@@ -224,14 +224,14 @@
 			mm = (MCContextMenuManager) chartCanvas.getContextMenu();
 
 			// FIXME: The lanes field is initialized by initializeChartConfiguration which is called by the super constructor. This is too indirect for SpotBugs to resolve and should be simplified.
-			lanes.updateContextMenu(mm);
-			lanes.updateContextMenu(MCContextMenuManager.create(chartLegend.getControl()));
+			lanes.updateContextMenu(mm, false);
+			lanes.updateContextMenu(MCContextMenuManager.create(chartLegend.getControl()), true);
 			buildChart();
 
 			addResultActions(form);
 			tableFilterComponent.loadState(state.getChild(METHOD_PROFILING_TABLE_FILTER));
 			form.getToolBarManager()
-					.add(ActionToolkit.action(() -> lanes.openEditLanesDialog(mm), Messages.ThreadsPage_EDIT_LANES,
+					.add(ActionToolkit.action(() -> lanes.openEditLanesDialog(mm, false), Messages.ThreadsPage_EDIT_LANES,
 							FlightRecorderUI.getDefault().getMCImageDescriptor(ImageConstants.ICON_LANES_EDIT)));
 			form.getToolBarManager().add(new Separator());
 			OrientationAction.installActions(form, sash);
--- a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/ThreadsPage.java	Thu Dec 13 16:47:07 2018 +0100
+++ b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/ThreadsPage.java	Mon Dec 17 16:51:30 2018 -0500
@@ -162,10 +162,10 @@
 			sash.setOrientation(SWT.HORIZONTAL);
 			mm.add(new Separator());
 			// FIXME: The lanes field is initialized by initializeChartConfiguration which is called by the super constructor. This is too indirect for SpotBugs to resolve and should be simplified.
-			lanes.updateContextMenu(mm);
+			lanes.updateContextMenu(mm, false);
 
 			form.getToolBarManager()
-					.add(ActionToolkit.action(() -> lanes.openEditLanesDialog(mm), Messages.ThreadsPage_EDIT_LANES,
+					.add(ActionToolkit.action(() -> lanes.openEditLanesDialog(mm, false), Messages.ThreadsPage_EDIT_LANES,
 							FlightRecorderUI.getDefault().getMCImageDescriptor(ImageConstants.ICON_LANES_EDIT)));
 			form.getToolBarManager().update(true);
 			chartLegend.getControl().dispose();