Bug#494: Fix for ability to set terminal tab name programmatically #508

mmx85 · 2023-08-15T14:12:17Z

setTerminalTitle method sets terminal title unconditionally. When you try to set title programmatically using the following code:
HashMap<String, Object> params = new HashMap<String, Object>();
params.put(ITerminalsConnectorConstants.PROP_TITLE, "some name"));
params.put(ITerminalsConnectorConstants.PROP_DELEGATE_ID, delegateId);
params.put(ITerminalsConnectorConstants.PROP_PROCESS_PATH, bashShellExecutablePath);
params.put(ITerminalsConnectorConstants.PROP_PROCESS_WORKING_DIR, workingDir);
For some terminals like Bash "\e]0;...\u0007" ANSI command is issued when terminal is initialized. It redefines the terminal name that is set by params.put(ITerminalsConnectorConstants.PROP_TITLE, "some name"));.

The proposed fix adds a new parameter "String requestor" for setTerminalTitle method to track originator of the change and a new property flag PROP_TITLE_UPDATE_API. When developer sets "true" for PROP_TITLE_UPDATE_API flag then ANSI command is ignored and terminal title is not updated when user executes "\e]0;\u0007" in terminal, but title can be changed from a pop-up menu or programmatically. If developer sets "false" for PROP_TITLE_UPDATE_API flag or does not set anything for the flag (in this case PROP_TITLE_UPDATE_API flag is set to "false" implicitly) then existing behavior is preserved.

The update allows to add different behavior for different originators in future by updating setTerminalTitle logic to process different requestors with minimal changes in the other places.

Fixes #494

jonahgraham · 2023-08-15T18:06:50Z

Code cleanliness issues fixed

:-) thanks! I am reviewing the change and have approved to run again.

There is a failure to build that looks like it may be due to a change in our dependencies:

Error: Failed to resolve target definition file:/home/runner/work/cdt/cdt/releng/org.eclipse.cdt.target/cdt.target: Could not find "javax.inject/0.0.0" in the repositories of the current location -> [Help 1]
Error:
Error: To see the full stack trace of the errors, re-run Maven with the -e switch.
Error: Re-run Maven using the -X switch to enable full debug logging.
Error:
Error: For more information about the errors and possible solutions, please read the following articles:
Error: [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MavenExecutionException
Error: Process completed with exit code 1.

I am also looking at that now.

jonahgraham · 2023-08-15T18:17:35Z

Thanks for fixing that other code cleanliness issue :-)

have approved to run again.

PS Once your first commit is merged into the repo the workflows will run automatically on future PRs. For now I will just continue to press Approve and Run as soon as I see the new commits.

mmx85 · 2023-08-15T18:26:13Z

Thanks @jonahgraham. Now another failure has happened. From console output it is unclear what exactly has happened.
BTW when I ty to setup build environment automatically following https://github.com/eclipse-cdt/cdt/blob/main/CONTRIBUTING.md#setup-cdt-for-development-with-oomph some error happens at the end of the process that prevents successful completion of dev environment setup.

jonahgraham · 2023-08-15T18:28:43Z

Now another failure has happened

That failure is the same as #508 (comment) - I have raised #509 and I am actively trying to get a fix rolled out for it now.

BTW when I ty to setup build environment automatically following https://github.com/eclipse-cdt/cdt/blob/main/CONTRIBUTING.md#setup-cdt-for-development-with-oomph some error happens at the end of the process that prevents successful completion of dev environment setup.

I'll have a look soon, tracking it in #511 if you have more info to add.

jonahgraham · 2023-08-15T18:55:39Z

I added Fixes #494 to the body of the message which causes GitHub to auto-link and auto-close the #494 issue once I merge this

I have applied the (probable) target platform fix to this branch too (from #495) so we can get workflows running.

...ipse.tm.terminal.control/src/org/eclipse/tm/internal/terminal/control/ITerminalListener.java

jonahgraham · 2023-08-15T19:03:48Z

....terminal.test/src/org/eclipse/tm/internal/terminal/emulator/MockTerminalControlForText.java

-	public void setTerminalTitle(String title) {
-		allTitles.add(title);
+	public void setTerminalTitle(String title, String requestor) {
+		if (requestor.equals("ANSI")) {


This constant should be shared so that future readers of the code can track down how these things connect.

Have you considered using an enum to handle the requestor?

Agree. What do you think is the best place for this enum?

The enum can go right next to the main declaration i.e. an inner enum in org.eclipse.tm.internal.terminal.emulator.VT100TerminalControl.setTerminalTitle(String, String)

be clear in the API if null is allowed for the requestor (you have it documented in the implementation class, but that needs to be in the interface because that is the API)

All the places that you updated to call the new API don't really seem necessary, e.g. you don't need to update the one calling with "RCM" at all with the evolved API.

jonahgraham · 2023-08-15T19:06:01Z

...clipse.tm.terminal.view.ui/src/org/eclipse/tm/terminal/view/ui/tabs/TabTerminalListener.java

+		CTabItem item = getTabItem();
+		if (item == null || item.isDisposed()) {
+			return;
+		}
+
+		// Run asynchronously in the display thread
+		item.getDisplay().asyncExec(() -> {
+			Boolean flag = false;
+			// Get the original terminal properties associated with the tab item
+			Map<String, Object> properties = (Map<String, Object>) item.getData("properties"); //$NON-NLS-1$
+			if (properties.containsKey(ITerminalsConnectorConstants.PROP_TITLE_UPDATE_API)) {
+				flag = (Boolean) properties.get(ITerminalsConnectorConstants.PROP_TITLE_UPDATE_API);
+			}
+			// Check if terminal should be updated either using API only (flag == true)
+			// or using API and ANSI command (flag == false).
+			if (flag == true && requestor != null && requestor.equals("ANSI")) {
+				return;
+			}


This looks mostly like the inlined version of updateTitle. You can refactor private methods so we don't need to duplicate this code.

jonahgraham

The change itself looks pretty good to me. We are bitten by this code being API and we cannot break public API.

Please let me know how much guidance you would like me to provide on how to handle evolving the API. I can leave you to it, answer questions, or even provide some code. Let me know what you need as I am keen to get this done, ideally by Monday so we can get it into 2023-09 M3 build next week.

jonahgraham · 2023-08-18T17:49:34Z

Great timing on pushing the new code - I am just now looking at terminal issues ahead of M3 build this weekend! Let me know when you are ready for me to review your updates.

jonahgraham · 2023-08-18T17:50:44Z

We are bitten by this code being API and we cannot break public API.

Since you first created this PR I have added a feature to the build so that the "Code Cleanliness Checks" verifies the API changes are ok in #514

mmx85 · 2023-08-18T19:11:47Z

Great that I managed to meet the deadline with new push. I have just fixed the formatting. Please review. I hope I understood your idea for fixing correctly and we are both on the same page.

mmx85 · 2023-08-18T20:00:09Z

I do not understand this error:
The following bundles are missing a service segment version bump:

org.eclipse.tm.terminal.view.ui
Please bump service segment by 100 if on main branch

When a more complete implementation of ANSI Escape sequence for renaming terminal titles was added in [CDT 10.2](https://github.com/eclipse-cdt/cdt/blob/main/NewAndNoteworthy/CDT-10.2.md#rename-terminal-tab) it caused a regression in use cases where extenders of the terminal wanted to retain control of the terminal's title. This commit adds a new flag that will prevent the title of the terminal tab from being updated from ANSI escape sequences. Fixes eclipse-cdt#494

jonahgraham

I have reviewed and applied the requested changes as I really want to get this in to 11.3.

Aside from the comments I made I also renamed the new flag as I found the wording confusing as to what it was doing. I hope that the rename isn't too disruptive to you.

I squashed your previous code into one commit (with a more expansive commit message) and added a new commit 7d6039a that has all my changes. I will squash that ahead of merging the change.

If you have a chance to look at this before (or even after) I merge that would be great. The build is running now and I will merge it before producing the M3 build assuming no blocking comments.

I do not understand this error: The following bundles are missing a service segment version bump:

org.eclipse.tm.terminal.view.ui
Please bump service segment by 100 if on main branch

Please see https://wiki.eclipse.org/Version_Numbering#When_to_change_the_service_segment for details. I have made the change.

jonahgraham · 2023-08-18T19:30:37Z

...rminal.test/src/org/eclipse/tm/internal/terminal/connector/TerminalConnectorFactoryTest.java

@@ -16,6 +16,8 @@
 *******************************************************************************/
 package org.eclipse.tm.internal.terminal.connector;

+import static org.eclipse.tm.internal.terminal.control.ITerminalListener3.*;


Throughout the code there are these import static statements. However they are unused and the save actions on the editor removes them.

I will push an update that handles this cleanup.

jonahgraham · 2023-08-18T19:34:17Z

...clipse.tm.terminal.view.ui/src/org/eclipse/tm/terminal/view/ui/tabs/TabTerminalListener.java

+	}
+
+	/**
+	 * Sets Terminal tilte and checks if originator is ANSI command.


There are a few typos that I am fixing as I am going - comment here is so you know what I am changing compared to your commit.

jonahgraham · 2023-08-18T19:36:10Z

...clipse.tm.terminal.view.ui/src/org/eclipse/tm/terminal/view/ui/tabs/TabTerminalListener.java

 		// Run asynchronously in the display thread
 		item.getDisplay().asyncExec(() -> {
+			Boolean flag = false;
+			// Get the original terminal properties associated with the tab item
+			final Map<String, Object> properties = (Map<String, Object>) item.getData("properties"); //$NON-NLS-1$


I see in other places in the code similar typecasts also exist. This line introduces a new warning which I will suppress with @SuppressWarnings({ "unchecked" })

jonahgraham · 2023-08-18T19:39:03Z

...clipse.tm.terminal.view.ui/src/org/eclipse/tm/terminal/view/ui/tabs/TabTerminalListener.java

+			}
+			// Check if terminal should be updated either using API only (flag == true)
+			// or using API and ANSI command (flag == false).
+			if (flag == true && requestor != null && requestor == TerminalTitleRequestor.ANSI) {


The == true and null check is not needed.

Suggested change

if (flag == true && requestor != null && requestor == TerminalTitleRequestor.ANSI) {

if (flag && requestor == TerminalTitleRequestor.ANSI) {

jonahgraham · 2023-08-18T19:43:04Z

...clipse.tm.terminal.view.ui/src/org/eclipse/tm/terminal/view/ui/tabs/TabTerminalListener.java

+			// Get the original terminal properties associated with the tab item
+			final Map<String, Object> properties = (Map<String, Object>) item.getData("properties"); //$NON-NLS-1$
+			if (properties.containsKey(ITerminalsConnectorConstants.PROP_TITLE_UPDATE_API)) {
+				flag = (Boolean) properties.get(ITerminalsConnectorConstants.PROP_TITLE_UPDATE_API);


This typecast is slightly unsafe - in TerminalService.java you correctly check the type. Following the style in org.eclipse.tm.terminal.view.ui.tabs.TabFolderMenuHandler.doCreateContextMenuActions().new TerminalActionPaste() {...}.run() means we should probably type check this.

Suggested change

flag = (Boolean) properties.get(ITerminalsConnectorConstants.PROP_TITLE_UPDATE_API);

if (properties.get(ITerminalsConnectorConstants.PROP_TITLE_UPDATE_API) instanceof Boolean flagObj) {

flag = flagObj;

}

jonahgraham · 2023-08-18T19:50:59Z

...clipse.tm.terminal.view.ui/src/org/eclipse/tm/terminal/view/ui/tabs/TabTerminalListener.java

 		// Run asynchronously in the display thread
 		item.getDisplay().asyncExec(() -> {
+			Boolean flag = false;
+			// Get the original terminal properties associated with the tab item
+			final Map<String, Object> properties = (Map<String, Object>) item.getData("properties"); //$NON-NLS-1$


Unfortunately this introduces an exception when closing tabs:

!ENTRY org.eclipse.ui 4 0 2023-08-18 15:47:32.497 !MESSAGE Unhandled event loop exception !STACK 0 org.eclipse.swt.SWTException: Widget is disposed at org.eclipse.swt.SWT.error(SWT.java:4918) at org.eclipse.swt.SWT.error(SWT.java:4833) at org.eclipse.swt.SWT.error(SWT.java:4804) at org.eclipse.swt.widgets.Widget.error(Widget.java:450) at org.eclipse.swt.widgets.Widget.checkWidget(Widget.java:369) at org.eclipse.swt.widgets.Widget.getData(Widget.java:544) at org.eclipse.tm.terminal.view.ui.tabs.TabTerminalListener.lambda$1(TabTerminalListener.java:125) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:132) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4047) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3663) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1155) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:342) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1046) at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:155) at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:645) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:342) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:552) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:171) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:203) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:136) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:402) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:568) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:651) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:588) at org.eclipse.equinox.launcher.Main.run(Main.java:1459) at org.eclipse.equinox.launcher.Main.main(Main.java:1432)

Suggested change

final Map<String, Object> properties = (Map<String, Object>) item.getData("properties"); //$NON-NLS-1$

if (item.isDisposed()) {

// tab has been closed since

}

final Map<String, Object> properties = (Map<String, Object>) item.getData("properties"); //$NON-NLS-1$

PS The reason this wasn't a problem before is the call to getTerminalConsoleTabTitle was returning null if item was disposed so the item.setText wasn't reachable in that condition

mmx85 · 2023-08-18T22:40:09Z

Thanks @jonahgraham. I'm OK with the changes you did. My initial idea for vars and consts naming was that we may want to disable not only ANSI command to update title but other, but eventually this all narrows to ANSI command only, for now.

jonahgraham · 2023-08-18T22:45:31Z

Ahh. I understand your naming. We can add additional consts as we need more options in the future. But if you have anything else you would like to add now we should try to get it in now., so feel free to let me know.

mmx85 · 2023-08-18T22:46:21Z

It would be much easier to work on fixes when #511 is resolved. Please prioritize this issue for resolving soon.

jonahgraham · 2023-08-18T23:02:39Z

#511 is a priority, but I'm held up by my number one priority which is the builds are all failing on Jenkins for the last few days!

mmx85 · 2023-08-19T07:32:17Z

But if you have anything else you would like to add now we should try to get it in now., so feel free to let me know.

No. What we have now is fine.

jonahgraham mentioned this pull request Aug 15, 2023

Eclipse CDT's Oomph setup not working #511

Closed

jonahgraham reviewed Aug 15, 2023

View reviewed changes

...ipse.tm.terminal.control/src/org/eclipse/tm/internal/terminal/control/ITerminalListener.java Show resolved Hide resolved

jonahgraham reviewed Aug 15, 2023

View reviewed changes

jonahgraham requested changes Aug 15, 2023

View reviewed changes

jonahgraham force-pushed the terminal-bug-494 branch from af7643e to 18634bb Compare August 16, 2023 00:07

maxoleksiv-ifx and others added 2 commits August 18, 2023 17:23

Code review changes

7d6039a

jonahgraham force-pushed the terminal-bug-494 branch from 684d8db to 7d6039a Compare August 18, 2023 21:25

jonahgraham approved these changes Aug 18, 2023

View reviewed changes

jonahgraham merged commit 422aea1 into eclipse-cdt:main Aug 19, 2023
3 of 4 checks passed

mmx85 deleted the terminal-bug-494 branch August 19, 2023 07:31

jonahgraham added this to the 11.3.0 milestone Sep 13, 2023

jonahgraham added the noteworthy Pull requests and fixed issues that should be highlighted to users label Sep 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug#494: Fix for ability to set terminal tab name programmatically #508

Bug#494: Fix for ability to set terminal tab name programmatically #508

mmx85 commented Aug 15, 2023 •

edited by jonahgraham

Loading

jonahgraham commented Aug 15, 2023

jonahgraham commented Aug 15, 2023

mmx85 commented Aug 15, 2023

jonahgraham commented Aug 15, 2023

jonahgraham commented Aug 15, 2023

jonahgraham Aug 15, 2023

mmx85 Aug 15, 2023

jonahgraham Aug 16, 2023

jonahgraham Aug 15, 2023

jonahgraham left a comment

jonahgraham commented Aug 18, 2023

jonahgraham commented Aug 18, 2023

mmx85 commented Aug 18, 2023

mmx85 commented Aug 18, 2023

jonahgraham left a comment

jonahgraham Aug 18, 2023

jonahgraham Aug 18, 2023

jonahgraham Aug 18, 2023

jonahgraham Aug 18, 2023

jonahgraham Aug 18, 2023

jonahgraham Aug 18, 2023

mmx85 commented Aug 18, 2023

jonahgraham commented Aug 18, 2023

mmx85 commented Aug 18, 2023

jonahgraham commented Aug 18, 2023

mmx85 commented Aug 19, 2023

	if (flag == true && requestor != null && requestor == TerminalTitleRequestor.ANSI) {
	if (flag && requestor == TerminalTitleRequestor.ANSI) {

-				flag = (Boolean) properties.get(ITerminalsConnectorConstants.PROP_TITLE_UPDATE_API);
+				if (properties.get(ITerminalsConnectorConstants.PROP_TITLE_UPDATE_API) instanceof Boolean flagObj) {
+					flag = flagObj;
+				}

Bug#494: Fix for ability to set terminal tab name programmatically #508

Bug#494: Fix for ability to set terminal tab name programmatically #508

Conversation

mmx85 commented Aug 15, 2023 • edited by jonahgraham Loading

jonahgraham commented Aug 15, 2023

jonahgraham commented Aug 15, 2023

mmx85 commented Aug 15, 2023

jonahgraham commented Aug 15, 2023

jonahgraham commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham commented Aug 18, 2023

jonahgraham commented Aug 18, 2023

mmx85 commented Aug 18, 2023

mmx85 commented Aug 18, 2023

jonahgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmx85 commented Aug 18, 2023

jonahgraham commented Aug 18, 2023

mmx85 commented Aug 18, 2023

jonahgraham commented Aug 18, 2023

mmx85 commented Aug 19, 2023

mmx85 commented Aug 15, 2023 •

edited by jonahgraham

Loading