Use ${ProjName} for workspace includes referencing folders from the project to better support project renames #402 #405

jantje · 2023-05-31T02:54:33Z

I don't really like this solution but it is the best I could come up with.
If there are better ways please tell me how.

I don't really like this but it is the best I could come up with

jonahgraham

This code has a bug if the user selects a project that starts with the same name as the current project you get an error. e.g. create two projects, simple and simpler. In simple press Add and then choose a directory in simpler, the resulting value you get is: /${ProjName}r/src/include instead of the expected /simpler/src/include

I think messing around with the string at this point is too late, instead I think the code that creates the string in the first place could be modified:

cdt/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/newui/AbstractCPropertyTab.java

Lines 576 to 582 in 7c8bb9f

    
           if (dialog.open() == Window.OK) { 
        
           	IResource resource = (IResource) dialog.getFirstResult(); 
        
           	if (resource != null) { 
        
           		StringBuilder buf = new StringBuilder(); 
        
           		return buf.append("${").append("workspace_loc:").append(resource.getFullPath()).append("}").toString(); //$NON-NLS-3$ //$NON-NLS-2$ //$NON-NLS-1$ 
        
           	} 
        
           }

using the same algorithm as discussed in #402 (comment)

If that proves impossible/impractical, the current solution looks fine to me, but does it need some null checks around get the project name from cfgd?

Please also provide a self contained commit message / PR title. If you add to the body of the commit message and PR something like Fixes #402 then merging the PR will close the issue with the appropriate cross reference. See help for more info on that.

jantje · 2023-05-31T20:39:36Z

I think messing around with the string at this point is too late, instead I think the code that creates the string in the first place could be modified:

I fully agree. That is why I stated : "I don't really like this solution but it is the best I could come up with."

I tried to modify cdt/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/newui/AbstractCPropertyTab.java
But the provided project is null.
So I tried getResDesc to get the project but the method is static so getResDesc is not available.
So I changed the method to not be static but it is called by static methods.
I didn't feel confident enough to remove the static from all required methods.

So I opted for a quick and dirty method.

If you are ok with removing the statics I'll do it.

jonahgraham · 2023-05-31T21:04:11Z

If it were me I would pass in the project as an additional parameter and leave it static.

jantje · 2023-05-31T21:14:58Z

A bit confusing as it already gets a IProject as a parameter

cdt/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/newui/AbstractCPropertyTab.java

Line 520 in 7c8bb9f

    
           private static String getWorkspaceDialog(Shell shell, String text, boolean dir, IProject prj) {

And a explicit null is provided and a check for null is done.

cdt/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/newui/AbstractCPropertyTab.java

Line 531 in 7c8bb9f

if (prj == null)

jonahgraham · 2023-05-31T21:46:36Z

Yeah, bit annoying to have two iproject in one signature. But they have distinctly different purposes at least.

Or add another boolean parameter like "limit selection to project" and always pass the project in.

The other alternative is to add a new button called project and that would limit selection to the project.

Or fix the bug in your initial implementation.

jantje · 2023-06-01T20:38:29Z

Or add another boolean parameter like "limit selection to project" and always pass the project in.

I'll try this one

This does not work because the project folder is added in front of the ${ProjectName}after pressing ok when having "is workspace" selected (which is default after pressing "add workspace folder")

jantje · 2023-06-01T22:14:28Z

This seems to fail as well as the project folder is added when getting back to the properties pages.
Suppose the project is called "C" and the folder "folder" You now get ${ProjName}/folder but then clicking OK again you get C/${ProjName}/folder
I tried to find where the project got added but I failed.

jonahgraham

First pass review before I try to run it.

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/newui/AbstractCPropertyTab.java

jonahgraham · 2023-06-02T00:30:53Z

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/newui/AbstractCPropertyTab.java

+		return getWorkspaceDialog(shell, text, false, true, null);
+	}
+
+	public static String getWorkspaceDirDialog(Shell shell, String text, IProject prj) {


Needs @since - but we can resolve that later in the review process

Which version number is to be used?

The minor version in MANIFEST.MF needs to be increased and use that number. The auto-fix in Eclipse should do that if you have API baseline setup. If you don't have it setup I think it will be 8.1

I can handle that as part of the merging if it isn't straightforward for you.

jonahgraham

The code change looks good - can you please address this:

Please also provide a self contained commit message / PR title. If you add to the body of the commit message and PR something like Fixes #402 then merging the PR will close the issue with the appropriate cross reference. See help for more info on that.

jonahgraham · 2023-06-06T18:11:36Z

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/newui/AbstractCPropertyTab.java

+		return getWorkspaceDialog(shell, text, false, true, null);
+	}
+
+	public static String getWorkspaceDirDialog(Shell shell, String text, IProject prj) {


The minor version in MANIFEST.MF needs to be increased and use that number. The auto-fix in Eclipse should do that if you have API baseline setup. If you don't have it setup I think it will be 8.1

I can handle that as part of the merging if it isn't straightforward for you.

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/newui/AbstractCPropertyTab.java

jantje · 2023-06-07T21:37:10Z

Fixes Rename project does change include directories pointing to this project #402
I do not really want to say this fixes 402 as it only makes the problem less annoying.
Basically a project rename listener(?) should handle includes which use the project name.
MANIFEST.MF
I remember having had these API incompatibility errors and quick fixes to fix them. But now no errors appear. That may be because I had to clone at GIT level. Anyway I would prefer you to do so as I do not know whether all manifests need updating or only the touched project.

jonahgraham · 2023-06-07T22:31:28Z

See https://github.com/eclipse-cdt/cdt/blob/main/POLICY.md#Using-API-Tooling for instructions on setting up and using API tooling

jonahgraham · 2023-06-07T22:34:07Z

I'm fixing API errors, doing last check and merging this now.

jonahgraham · 2023-06-07T23:02:24Z

I found a bug related to how the new code was handling slashes if the project was the selection as opposed to a folder in a project. In commit bde0761 I fixed the handling to use Path class to assemble the path as the reference code did that I mentioned in #402 (comment)

jonahgraham · 2023-06-07T23:26:16Z

I do not really want to say this fixes 402 as it only makes the problem less annoying.

I understand what you mean - for now it is marked as fixed, at least fixed in the same way as other places using the same methodology.

A solution for eclipse-cdt#402

bb1f29d

I don't really like this but it is the best I could come up with

jonahgraham requested changes May 31, 2023

View reviewed changes

Did the changes as discussed but does not work eclipse-cdt#402

32b3345

This does not work because the project folder is added in front of the ${ProjectName}after pressing ok when having "is workspace" selected (which is default after pressing "add workspace folder")

jonahgraham requested changes Jun 2, 2023

View reviewed changes

Use absolute path and isEqual as per review request eclipse-cdt#402

71a52d0

jantje changed the title ~~A solution for #402~~ Fix for #402 Jun 4, 2023

jonahgraham requested changes Jun 6, 2023

View reviewed changes

jonahgraham added this to the 11.3.0 milestone Jun 6, 2023

jantje changed the title ~~Fix for #402~~ Use ${ProjName} for workspace includes referencing folders from the project to better support project renames #402 Jun 7, 2023

jonahgraham linked an issue Jun 7, 2023 that may be closed by this pull request

Rename project does change include directories pointing to this project #402

Closed

Fix API tags and extra slash

bde0761

jonahgraham enabled auto-merge (squash) June 7, 2023 23:24

jonahgraham approved these changes Jun 7, 2023

View reviewed changes

jonahgraham merged commit 501e7db into eclipse-cdt:main Jun 7, 2023

jonahgraham mentioned this pull request Jun 7, 2023

Rename project does change include directories pointing to this project #402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ${ProjName} for workspace includes referencing folders from the project to better support project renames #402 #405

Use ${ProjName} for workspace includes referencing folders from the project to better support project renames #402 #405

jantje commented May 31, 2023

jonahgraham left a comment

jantje commented May 31, 2023

jonahgraham commented May 31, 2023

jantje commented May 31, 2023

jonahgraham commented May 31, 2023

jantje commented Jun 1, 2023

jantje commented Jun 1, 2023

jonahgraham left a comment

jonahgraham Jun 2, 2023

jantje Jun 4, 2023

jonahgraham Jun 6, 2023

jonahgraham left a comment

jonahgraham Jun 6, 2023

jantje commented Jun 7, 2023

jonahgraham commented Jun 7, 2023

jonahgraham commented Jun 7, 2023

jonahgraham commented Jun 7, 2023

jonahgraham commented Jun 7, 2023

	if (dialog.open() == Window.OK) {
	IResource resource = (IResource) dialog.getFirstResult();
	if (resource != null) {
	StringBuilder buf = new StringBuilder();
	return buf.append("${").append("workspace_loc:").append(resource.getFullPath()).append("}").toString(); //$NON-NLS-3$ //$NON-NLS-2$ //$NON-NLS-1$
	}
	}

Use ${ProjName} for workspace includes referencing folders from the project to better support project renames #402 #405

Use ${ProjName} for workspace includes referencing folders from the project to better support project renames #402 #405

Conversation

jantje commented May 31, 2023

jonahgraham left a comment

Choose a reason for hiding this comment

jantje commented May 31, 2023

jonahgraham commented May 31, 2023

jantje commented May 31, 2023

jonahgraham commented May 31, 2023

jantje commented Jun 1, 2023

jantje commented Jun 1, 2023

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham Jun 2, 2023

Choose a reason for hiding this comment

jantje Jun 4, 2023

Choose a reason for hiding this comment

jonahgraham Jun 6, 2023

Choose a reason for hiding this comment

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham Jun 6, 2023

Choose a reason for hiding this comment

jantje commented Jun 7, 2023

jonahgraham commented Jun 7, 2023

jonahgraham commented Jun 7, 2023

jonahgraham commented Jun 7, 2023

jonahgraham commented Jun 7, 2023