GDB Remote launch targets load attributes when editing.

[ewaterlander]

When a GDB Remote TCP or Serial launch target was opened for editing the current attribute values were not shown.

GDB Remote TCP always looks like this when you edit it:

GDB Remote Serial always looks like this:

[ewaterlander]

@betamaxbandit , would you like to review this?

[betamaxbandit]

Hi @ewaterlander ,
After creating a gdb remote serial or tcp launch target, I cannot see them in launch target list in the launch bar. Can you remind me what type of project I need to create?
It would be helpful to add some manual test instructions for testing this change I think.

[ewaterlander]

Can you remind me what type of project I need to create?

First create a project. CMake is fine.
Open the Debug Configurations wizard via Run > Debug Configurations...
Create a new C/C++ Remote Application launch configuration for your project.

In the main tab, at the bottom, you see that the default selected is "Automatic Debugging Launcher". Press "Select other..."

In the next wizard select "Use configuration specific settings"

Select Manual Remote Debugging Launcher.
Press OK.
Press Close

You now have a simple launch configuration which needs just a server name and port number.
Select the new configuration in the launchbar and create a Gdb Remote TCP launch target.
The Gdb Remote Serial is also compatible with this launch configuration.

In the Debugger tab of the new Launch configuration, in sub-tab Connection, you can select if you want to connect via TCP or Serial.

[betamaxbandit]

I've remembered how to view a created serial/tcp launch target. From the launch bar launch configuration dropdown, choose New Launch Configuration... and choose C/C++ Remote Application and finish the wizard.
Select this launch configuration and any serial/tcp launch targets should be visible in the launch target dropdown.

This is a summary of my comments. Many of them are general and are about improvements that could be made to these wizards. So either do them in this change or they could be done in subsequent changes.
Happy to discuss in more detail.

1. Error shown immediately
   When the new "GDB Remote TCP" launch target wizard is launched, it immediately shows the error message:
   "Hostname or IP must be specified"
   This is against Eclipse wizard design guidelines;
   Guideline 5.3
   Start the wizard with a prompt, not an error message.
   https://eclipse-platform.github.io/ui-best-practices/#wizards

2. Not allow invalid chars and spaces.
   An error should be displayed if the user has chosen an invalid name.
   A target name with invalid chars actually gets fixed in ICBuildConfigurationProvider.getCBuildConfigName(IProject, String, IToolChain, String, ILaunchTarget), by calling LaunchTargetUtils.sanitizeName(String).
   However I think the launch target wizard should pick this up and not allow Finish until name is valid.

3. Not allow duplicate names.
   An error should be displayed if the user has chosen a name that already exists.
   In my wizard, I handle this:

List fExistingRCarLaunchTargetNames;

In page constructor:
fExistingRCarLaunchTargetNames = getExistingLaunchTargetNames();

/**
 * Used to create a cache of existing launch target names so it is quick to check if a new name already exists.
 */
private List<String> getExistingLaunchTargetNames() {
	List<String> retVal = new ArrayList<>();
	ILaunchTargetManager manager = RcarLaunchTargetUIPlugin.getService(ILaunchTargetManager.class);
	if (manager != null) {
		retVal = Arrays.stream(manager.getLaunchTargetsOfType( LAUNCH_TARGET_TYPE_ID))
				.map(ILaunchTarget::getId)
				.collect(Collectors.toList());
	}
	return retVal;
}

where LAUNCH_TARGET_TYPE_ID is the type like GDBRemoteSerialLaunchTargetProvider.TYPE_ID or GDBRemoteTCPLaunchTargetProvider.TYPE_ID

and then in a validate method:

	if (fIsCreating && fExistingRCarLaunchTargetNames.contains(name)) {
		setErrorMessage("A launch target with that name already exists.");
		return false;
	}  

4. Not possible to delete
   In "GDB Remote Serial" launch target wizard lacks delete button, so once a target is created, it is not possible for the user to delete a target.
   It is possible to delete the TCP target however.
   I think if the user has the ability to choose a name for something, then they should be allowed to delete it.

[betamaxbandit]

I think there is a better way to deal with name. I would not introduce a new property called "name" here. I'd stick to using the id. I expect you did introduce it so that you could set a name in createControl.

I think a better way is to get the launch target target (LaunchTargetWizard.getLaunchTarget()) and pass it as a param to in addPages(); see line 195.

hmm - I can't figure out how to add a comment on line 195 - maybe because it was changed?? Anyway here's my code suggestion:

public void addPages() {
	super.addPages();
	addPage(new SerialPage(getLaunchTarget()));
}

Then in the SerialPage class you can stash the launch target. When the user is creating a new launch target it will be null. But if they are edting/viewing an existing one, it will contain values and it's id will be set.

When the launch target already exists (user can edit it), in my wizards, I have disabled the name field so it cannot be changed, for example:

	/*
	 * If editing an existing launch target, then the name field cannot be edited.
	 */
	nameText.setEnabled(fIsCreating);

[ewaterlander]

I think there is a better way to deal with name. I would not introduce a new property called "name" here. I'd stick to using the id. I expect you did introduce it so that you could set a name in createControl.

I did not introduce the "name" attribute, it was already there. I first noticed it when I inspected file workspace file ./.metadata/.plugins/org.eclipse.core.runtime/.settings/org.eclipse.launchbar.core.prefs which stores all the launch targets' attributes. I saw the "name" attribute was not updated when the user changes the name of the launch target.

The "name" attribute is added to every launch target when it is created by the LaunchTargetManager.
See LaunchTargetManager.addLaunchTargetNoNotify(), at line 214.
The id is stored as attribute "name", just to have at least one attribute.

We could ignore the "name" attribute, because I think it is not used anywhere in the CDT code, but then it will have an incorrect value if a user changes the name and somebody does use the attribute.
Perhaps its better that the method that stores the launch target will always write the "name" attribute with the id value.

if (prefs.nodeExists(childName)) {
	child = prefs.node(childName);
} else {
	child = prefs.node(childName);
	// set the id so we have at least one attribute to save
	child.put("name", id); //$NON-NLS-1$
}
ILaunchTarget target = new LaunchTarget(typeId, id, child);

[betamaxbandit]

Can you remind me what type of project I need to create?

First create a project. CMake is fine. Open the Debug Configurations wizard via Run > Debug Configurations... Create a new C/C++ Remote Application launch configuration for your project.

Thanks, only just seen this. I was in the middle of writing my review ☺️

[ewaterlander]

This is a summary of my comments. Many of them are general and are about improvements that could be made to these wizards. So either do them in this change or they could be done in subsequent changes. Happy to discuss in more detail.

Thanks for your points. I'm also happy to discuss about them in more detail, but I prefer to solve those issues step by step in subsequent changes.

In this PR I would like to only fix the restoring of values. I think it's the most annoying problem at the moment.

[ewaterlander]

Hi,
The wizards don't use or write the "name" attribute anymore.
The "name" attribute is now always saved consistently with the id.

[betamaxbandit]

Hi @ewaterlander ,
this looks better now!

You'll need to update the copyright header on all the java files you've changed. I think just add ", 2025"

Also do you intend adding any tests; automatic or manual? Tests don't seem to be mandatory. Not sure if this is the best approach, but I've added manual tests before, like this:

build/org.eclipse.cdt.build.gcc.core.tests/manualTests/README.md

Or there are some SWTBot tests in some of the test bundles. Might be overkill though.

p.s. I note you've added this issue for the other comments I had:
Cleanup Gdb Remote launch targets. #1232
#1232

[ewaterlander]

Hi @betamaxbandit

I have updated the Copyright years.

Also do you intend adding any tests; automatic or manual? Tests don't seem to be mandatory. Not sure if this is the best approach, but I've added manual tests before, like this:

Or there are some SWTBot tests in some of the test bundles. Might be overkill though.

I did manual testing. I could describe that.

I have no experience with SWTBot tests. This is a nice opportunity to learn. It would be nice to do it with a CoreBuild project, but CoreBuild projects do not support GdbRemote targets yet. For this we need #1222 to be merged first.

I will start with a SWTBot test. I could add it to this change, or do it in a subsequent change.

[betamaxbandit]

LGTM

[betamaxbandit]

Hi @ewaterlander

I will start with a SWTBot test. I could add it to this change, or do it in a subsequent change.

Awesome!

I haven't done much SWTBot myself. This is my most recent effort, which could help:

#879

[betamaxbandit]

Hi @ewaterlander ,
I've merged this now.
I understand you may be able to submit an SWTBot test in a subsequent change.

[ewaterlander]

Hi @ewaterlander , I've merged this now. I understand you may be able to submit an SWTBot test in a subsequent change.

Hi @betamaxbandit , thanks for merging. Yes, I will work on a SWTBot test.