eclipse.platform.ui icon indicating copy to clipboard operation
eclipse.platform.ui copied to clipboard

case-sensitive file existence check

Open andrew-tram opened this issue 1 year ago • 11 comments

Check to address - https://github.com/eclipse-platform/eclipse.platform.ui/issues/1866

andrew-tram avatar Jun 18 '24 15:06 andrew-tram

@jukzi , good call! I adjusted the checks to account for OS

andrew-tram avatar Jun 18 '24 15:06 andrew-tram

Understood, hopefully this is closer...

	    // Use workspace and resource APIs to handle file comparison
	    IWorkspaceRoot root = ResourcesPlugin.getWorkspace().getRoot();
	    IPath newFilePath = getContainerFullPath().append(getFileName());
	    IFile newFileHandle = createFileHandle(newFilePath);

	    IFile[] files = root.findFilesForLocationURI(newFileHandle.getLocationURI());
	    for (IFile file : files) {
	        if (file.exists() && file.getName().equalsIgnoreCase(newFileHandle.getName()) &&
	                !file.getName().equals(newFileHandle.getName())) {
	            setErrorMessage(NLS.bind(IDEWorkbenchMessages.ResourceGroup_nameExistsDifferentCase, file.getName()));
	            return false;
	        }
	    }

andrew-tram avatar Jun 18 '24 15:06 andrew-tram

You should not need to compare Strings yourself.

jukzi avatar Jun 18 '24 15:06 jukzi

I’d expect to see IFile or IPath to be compared which know whether to do case-sensitive comparisons.

merks avatar Jun 18 '24 15:06 merks

I believe this is closer to what yall are suggesting...

// Use workspace and resource APIs to handle file comparison
IPath newFilePath = getContainerFullPath().append(getFileName());
IFile newFileHandle = createFileHandle(newFilePath);

try {
	IContainer parentContainer = newFileHandle.getParent();
	IResource[] members = parentContainer.members();
	for (IResource member : members) {
		if (member.getType() == IResource.FILE) {
			IPath memberPath = member.getFullPath();
			if (memberPath.equals(newFilePath)) {
				continue; // Skip if it's the same path
			}
			if (memberPath.lastSegment().equalsIgnoreCase(newFileHandle.getName()) &&
					!memberPath.lastSegment().equals(newFileHandle.getName())) {
				setErrorMessage(NLS.bind(IDEWorkbenchMessages.ResourceGroup_nameExistsDifferentCase, member.getName()));
				return false;
			}
		}
	}
} catch (CoreException e) {
	setErrorMessage("Error while validating file name: " + e.getMessage());
	return false;
}

Will externalize the new string if all is good.

andrew-tram avatar Jun 18 '24 15:06 andrew-tram

I think you should test it. I think memberPath.equals(newFilePath) will return true for two paths with different cases on a case-insensitive file system. I think you if want to check if two paths have different case, you should compare the strings. So if the paths are equal but the string representation are not equal, then what's specified is different from what's present. And that problem can be the case for the any segment of the path (project/folder), not just the final segment.

merks avatar Jun 18 '24 16:06 merks

Ive tested this on Windows 11 and Mac 14.4.1 (default HFS+ or APFS in case-insensitive mode). The expected results look good where an error displays saying the file already exists with the same name, different case.

Trying Linux now.

andrew-tram avatar Jun 18 '24 18:06 andrew-tram

So, I'm not entirely sure how it's feasible to do this without checking for at least if it's Linux.

andrew-tram avatar Jun 18 '24 21:06 andrew-tram

If this approach becomes a challenge, a smaller, targeted fix might be to not selectAndReveal the file and open it an editor if what is returned from mainPage.createNewFile() doesn't exist.

BasicNewFileResourceWizard:

	@Override
	public boolean performFinish() {
		IFile file = mainPage.createNewFile();
		if (file == null) {
			return false;
		}

mainPage.createNewFile() will trigger the display of the error message dialog showing "A resource exists with a different case", but it will return a non-null File object whose .exists() method would return false, but is not checked before invoking IDE.openEditor(page, file, true); just below

So perhaps just changing if (file == null) { to if (file == null || !file.exists()) { would dullen the bulk of the annoyance of https://github.com/eclipse-platform/eclipse.platform.ui/issues/1866

jwflicker avatar Jun 19 '24 00:06 jwflicker

You are supposed to find an existing method of signature boolean equals(IFile, IFile) or boolean equals(IPath, IPath) which does the OS check for you. I dont know its place or name by heart but am sure you can find many of them. Even java.nio.Path.equals(Path) probably does it if the Path is constructed using the appropriate FileSystem.

jukzi avatar Jun 19 '24 05:06 jukzi

Test Results

 1 210 files   -   605   1 210 suites   - 605   1h 4m 3s :stopwatch: - 31m 9s  7 661 tests ±    0   7 430 :white_check_mark:  -     3  231 :zzz: +  3  0 :x: ±0  16 096 runs   - 8 048  15 583 :white_check_mark:  - 7 812  513 :zzz:  - 236  0 :x: ±0 

Results for commit 805586d6. ± Comparison against base commit 954bc581.

This pull request skips 3 tests.
UiTestSuite org.eclipse.ui.tests.api.ApiTestSuite org.eclipse.ui.tests.api.WorkbenchPluginTest ‑ testGetImageRegistryFromAdditionalDisplay
org.eclipse.jface.text.tests.contentassist.ContextInformationTest ‑ testContextInfo_hide_focusOut
org.eclipse.urischeme.internal.registration.TestUnitWinRegistry ‑ testWinRegistry

github-actions[bot] avatar Jun 19 '24 06:06 github-actions[bot]