From e44ef41078ca8ffa985d7b2b8747d23bc51146a0 Mon Sep 17 00:00:00 2001
From: Jonah Graham <jonah@kichwacoders.com>
Date: Wed, 19 May 2021 12:48:23 -0400
Subject: [PATCH] Bug 573646: Restore to correct secondary tab

The handling of ssecondary ids was problematic, special cases were
not well documented and depending on code paths, a secondary ID
of null would mean different things.

This commit fixes that documentation, defines shared constants, and
fixes the logic so secondary id dispayed terminals restore to the
correct location on restart.

Change-Id: Ia87b64f735a37d505e1a5a7b7a4a8e210dfb8c1e
---
 .../META-INF/MANIFEST.MF                      |  2 +-
 .../ITerminalsConnectorConstants.java         | 25 +++++++++
 .../META-INF/MANIFEST.MF                      |  4 +-
 .../view/ui/manager/ConsoleManager.java       | 51 +++++++++----------
 .../view/ui/services/TerminalService.java     | 10 ++--
 5 files changed, 57 insertions(+), 35 deletions(-)

diff --git a/terminal/plugins/org.eclipse.tm.terminal.view.core/META-INF/MANIFEST.MF b/terminal/plugins/org.eclipse.tm.terminal.view.core/META-INF/MANIFEST.MF
index 19c8c7373fd..9f15d97d265 100644
--- a/terminal/plugins/org.eclipse.tm.terminal.view.core/META-INF/MANIFEST.MF
+++ b/terminal/plugins/org.eclipse.tm.terminal.view.core/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.tm.terminal.view.core;singleton:=true
-Bundle-Version: 4.7.100.qualifier
+Bundle-Version: 4.8.0.qualifier
 Bundle-Activator: org.eclipse.tm.terminal.view.core.activator.CoreBundleActivator
 Bundle-Vendor: %providerName
 Require-Bundle: org.eclipse.core.expressions;bundle-version="3.4.400",
diff --git a/terminal/plugins/org.eclipse.tm.terminal.view.core/src/org/eclipse/tm/terminal/view/core/interfaces/constants/ITerminalsConnectorConstants.java b/terminal/plugins/org.eclipse.tm.terminal.view.core/src/org/eclipse/tm/terminal/view/core/interfaces/constants/ITerminalsConnectorConstants.java
index ed14256fce1..c94d0262618 100644
--- a/terminal/plugins/org.eclipse.tm.terminal.view.core/src/org/eclipse/tm/terminal/view/core/interfaces/constants/ITerminalsConnectorConstants.java
+++ b/terminal/plugins/org.eclipse.tm.terminal.view.core/src/org/eclipse/tm/terminal/view/core/interfaces/constants/ITerminalsConnectorConstants.java
@@ -32,10 +32,35 @@ public interface ITerminalsConnectorConstants {
 	/**
 	 * Property: The unique secondary id of the terminals view to open.
 	 * <p>
+	 * Special values:
+	 * <ul>
+	 * <li>
+	 * not present in properties table or {@link #LAST_ACTIVE_SECONDARY_ID} means open on most recent terminal view
+	 * </li>
+	 * <li>
+	 * {@link #ANY_ACTIVE_SECONDARY_ID} means open on any terminal view
+	 * </li>
+	 * <li>
+	 * <code>null</code> means open on the first primary terminal (the one with no secondary id)
+	 * </li>
+	 * <ul>
+	 * <p>
 	 * Property Type: {@link String}
 	 */
 	public static final String PROP_SECONDARY_ID = "secondaryId"; //$NON-NLS-1$
 
+	/**
+	 * Special value for {@link #PROP_SECONDARY_ID} to indicate reuse of the most recent terminal view
+	 * @since 4.8
+	 */
+	public static final String LAST_ACTIVE_SECONDARY_ID = "last"; //$NON-NLS-1$
+
+	/**
+	 * Special value for {@link #PROP_SECONDARY_ID} to indicate reuse of any terminal view
+	 * @since 4.8
+	 */
+	public static final String ANY_ACTIVE_SECONDARY_ID = "*"; //$NON-NLS-1$
+
 	/**
 	 * Property: The title of the terminal tab to open.
 	 * <p>
diff --git a/terminal/plugins/org.eclipse.tm.terminal.view.ui/META-INF/MANIFEST.MF b/terminal/plugins/org.eclipse.tm.terminal.view.ui/META-INF/MANIFEST.MF
index 0d4a0192d6b..d5a95f4f69f 100644
--- a/terminal/plugins/org.eclipse.tm.terminal.view.ui/META-INF/MANIFEST.MF
+++ b/terminal/plugins/org.eclipse.tm.terminal.view.ui/META-INF/MANIFEST.MF
@@ -11,8 +11,8 @@ Require-Bundle: org.eclipse.core.expressions;bundle-version="3.4.400",
  org.eclipse.core.variables;bundle-version="3.2.600",
  org.eclipse.debug.ui;bundle-version="3.8.1";resolution:=optional,
  org.eclipse.egit.ui;bundle-version="2.0.0";resolution:=optional,
- org.eclipse.tm.terminal.view.core;bundle-version="4.5.0",
- org.eclipse.tm.terminal.control;bundle-version="4.5.0",
+ org.eclipse.tm.terminal.view.core;bundle-version="[4.8.0,5.0.0)",
+ org.eclipse.tm.terminal.control;bundle-version="[5.2.0,6.0.0)",
  org.eclipse.ui;bundle-version="3.8.0",
  org.eclipse.ui.ide;bundle-version="3.18.0";resolution:=optional,
  org.eclipse.ui.editors;bundle-version="3.14.0";resolution:=optional,
diff --git a/terminal/plugins/org.eclipse.tm.terminal.view.ui/src/org/eclipse/tm/terminal/view/ui/manager/ConsoleManager.java b/terminal/plugins/org.eclipse.tm.terminal.view.ui/src/org/eclipse/tm/terminal/view/ui/manager/ConsoleManager.java
index 072b4673582..428ebf5a5f2 100644
--- a/terminal/plugins/org.eclipse.tm.terminal.view.ui/src/org/eclipse/tm/terminal/view/ui/manager/ConsoleManager.java
+++ b/terminal/plugins/org.eclipse.tm.terminal.view.ui/src/org/eclipse/tm/terminal/view/ui/manager/ConsoleManager.java
@@ -17,6 +17,7 @@ import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 
 import org.eclipse.core.runtime.Assert;
 import org.eclipse.core.runtime.IStatus;
@@ -53,9 +54,6 @@ import org.eclipse.ui.PlatformUI;
  */
 public class ConsoleManager {
 
-	// Constant to indicate any secondary id is acceptable
-	private final static String ANY_SECONDARY_ID = new String("*"); //$NON-NLS-1$
-
 	// Reference to the perspective listener instance
 	private final IPerspectiveListener perspectiveListener;
 
@@ -198,7 +196,9 @@ public class ConsoleManager {
 	 * <b>Note:</b> The method must be called within the UI thread.
 	 *
 	 * @param id The terminals console view id or <code>null</code> to show the default terminals console view.
-	 * @param secondaryId The terminals console view secondary id or <code>null</code>.
+	 * @param secondaryId The terminal console secondary id, which may be <code>null</code> which is the secondary id of
+	 *        the first terminal view opened. To specify reuse of most recent terminal view use special value of
+	 *        {@link ITerminalsConnectorConstants#LAST_ACTIVE_SECONDARY_ID}.
 	 *
 	 * @return The console view instance if available or <code>null</code> otherwise.
 	 */
@@ -225,7 +225,9 @@ public class ConsoleManager {
 	 * Search and return a terminal view with a specific secondary id
 	 *
 	 * @param id The terminals console view id. Must not be <code>null</code>.
-	 * @param secondaryId The terminals console view secondary id or <code>null</code>.
+	 * @param secondaryId The terminal console secondary id, which may be <code>null</code> which is the secondary id of
+	 *        the first terminal view opened. To specify reuse of any active terminal view use special value of
+	 *        {@link ITerminalsConnectorConstants#ANY_ACTIVE_SECONDARY_ID}.
 	 * @param restore <code>True</code> if to try to restore the view, <code>false</code> otherwise.
 	 *
 	 * @return The terminals console view instance or <code>null</code> if not found.
@@ -235,8 +237,8 @@ public class ConsoleManager {
 
 		for (IViewReference ref : getActiveWorkbenchPage().getViewReferences()) {
 			if (ref.getId().equals(id)) {
-				if (ANY_SECONDARY_ID.equals(secondaryId) || secondaryId == null && ref.getSecondaryId() == null
-						|| secondaryId != null && secondaryId.equals(ref.getSecondaryId())) {
+				if (ITerminalsConnectorConstants.ANY_ACTIVE_SECONDARY_ID.equals(secondaryId)
+						|| Objects.equals(secondaryId, ref.getSecondaryId())) {
 					return ref.getView(restore);
 				}
 			}
@@ -248,7 +250,9 @@ public class ConsoleManager {
 	 * Search and return the active terminals view.
 	 *
 	 * @param id The terminals console view id. Must not be <code>null</code>.
-	 * @param secondaryId The terminals console view secondary id or <code>null</code>.
+	 * @param secondaryId The terminal console secondary id, which may be <code>null</code> which is the secondary id of
+	 *        the first terminal view opened. To specify reuse of most recent terminal view use special value of
+	 *        {@link ITerminalsConnectorConstants#LAST_ACTIVE_SECONDARY_ID}.
 	 * @return The terminals console view instance or <code>null</code> if not found.
 	 */
 	private IViewPart getActiveTerminalsView(String id, String secondaryId) {
@@ -257,8 +261,8 @@ public class ConsoleManager {
 		IViewPart part = null;
 
 		if (id.equals(lastActiveViewId)) {
-			if (secondaryId == null || ANY_SECONDARY_ID.equals(secondaryId)
-					|| secondaryId.equals(lastActiveSecondaryViewId)) {
+			if (ITerminalsConnectorConstants.LAST_ACTIVE_SECONDARY_ID.equals(secondaryId)
+					|| Objects.equals(secondaryId, lastActiveSecondaryViewId)) {
 				part = getTerminalsViewWithSecondaryId(lastActiveViewId, lastActiveSecondaryViewId, false);
 			}
 		}
@@ -363,7 +367,8 @@ public class ConsoleManager {
 			IViewPart activePart = getActiveTerminalsView(id != null ? id : IUIConstants.ID, secondaryId);
 			if (activePart == null) {
 				// Create a new one
-				IViewPart newPart = showConsoleView(id != null ? id : IUIConstants.ID, getSecondaryId(secondaryId, id));
+				IViewPart newPart = showConsoleView(id != null ? id : IUIConstants.ID,
+						getNextTerminalSecondaryId(IUIConstants.ID));
 				return newPart;
 			}
 
@@ -377,20 +382,6 @@ public class ConsoleManager {
 		return null;
 	}
 
-	/**
-	 * Return the secondary id to use.
-	 * @param secondaryId
-	 * @param id
-	 * @return the secondaryId argument is not null, or *, otherwise use the auto generated secondary id.
-	 */
-	private String getSecondaryId(String secondaryId, String id) {
-		if (secondaryId == null || ANY_SECONDARY_ID.equals(secondaryId)) {
-			return getNextTerminalSecondaryId(id != null ? id : IUIConstants.ID);
-		}
-
-		return secondaryId;
-	}
-
 	/**
 	 * Opens the console with the given title and connector.
 	 * <p>
@@ -402,10 +393,14 @@ public class ConsoleManager {
 	 * @param connector The terminal connector. Must not be <code>null</code>.
 	 * @param data The custom terminal data node or <code>null</code>.
 	 * @param flags The flags controlling how the console is opened or <code>null</code> to use defaults.
+	 * @deprecated Use {@link #openConsole(String, String, String, String, ITerminalConnector, Object, Map)}
+	 * and explicitly specify an ID {@link ITerminalsConnectorConstants#LAST_ACTIVE_SECONDARY_ID}
 	 */
+	@Deprecated
 	public CTabItem openConsole(String id, String title, String encoding, ITerminalConnector connector, Object data,
 			Map<String, Boolean> flags) {
-		return openConsole(id, ANY_SECONDARY_ID, title, encoding, connector, data, flags);
+		return openConsole(id, ITerminalsConnectorConstants.LAST_ACTIVE_SECONDARY_ID, title, encoding, connector, data,
+				flags);
 	}
 
 	/**
@@ -414,7 +409,9 @@ public class ConsoleManager {
 	 * <b>Note:</b> The method must be called within the UI thread.
 	 *
 	 * @param id The terminals console view id or <code>null</code> to show the default terminals console view.
-	 * @param secondaryId The terminals console view secondary id or <code>null</code>.
+	 * @param secondaryId The terminal console secondary id, which may be <code>null</code> which is the secondary id of
+	 *        the first terminal view opened. To specify reuse of most recent terminal view use special value of
+	 *        {@link ITerminalsConnectorConstants#LAST_ACTIVE_SECONDARY_ID}.
 	 * @param title The console title. Must not be <code>null</code>.
 	 * @param encoding The terminal encoding or <code>null</code>.
 	 * @param connector The terminal connector. Must not be <code>null</code>.
diff --git a/terminal/plugins/org.eclipse.tm.terminal.view.ui/src/org/eclipse/tm/terminal/view/ui/services/TerminalService.java b/terminal/plugins/org.eclipse.tm.terminal.view.ui/src/org/eclipse/tm/terminal/view/ui/services/TerminalService.java
index a6ee782d4ec..c5b8bf263b5 100644
--- a/terminal/plugins/org.eclipse.tm.terminal.view.ui/src/org/eclipse/tm/terminal/view/ui/services/TerminalService.java
+++ b/terminal/plugins/org.eclipse.tm.terminal.view.ui/src/org/eclipse/tm/terminal/view/ui/services/TerminalService.java
@@ -153,6 +153,9 @@ public class TerminalService implements ITerminalService {
 		// Extract the properties
 		String id = (String) properties.get(ITerminalsConnectorConstants.PROP_ID);
 		String secondaryId = (String) properties.get(ITerminalsConnectorConstants.PROP_SECONDARY_ID);
+		if (!properties.containsKey(ITerminalsConnectorConstants.PROP_SECONDARY_ID)) {
+			secondaryId = ITerminalsConnectorConstants.LAST_ACTIVE_SECONDARY_ID;
+		}
 		String title = (String) properties.get(ITerminalsConnectorConstants.PROP_TITLE);
 		Object data = properties.get(ITerminalsConnectorConstants.PROP_DATA);
 
@@ -292,11 +295,8 @@ public class TerminalService implements ITerminalService {
 				}
 				// Open the new console
 				CTabItem item;
-				if (secondaryId != null)
-					item = ConsoleManager.getInstance().openConsole(id, secondaryId, title, encoding, connector, data,
-							flags);
-				else
-					item = ConsoleManager.getInstance().openConsole(id, title, encoding, connector, data, flags);
+				item = ConsoleManager.getInstance().openConsole(id, secondaryId, title, encoding, connector, data,
+						flags);
 				// Associate the original terminal properties with the tab item.
 				// This makes it easier to persist the connection data within the memento handler
 				if (item != null && !item.isDisposed())