Use of tryPerformLTKcopy changes behavior of Subclass MoveFilesAndFoldersOpertation

[hd42]

getUndoableCopyOrMoveOperation is overwritten in MoveFilesAndFoldersOperation.

But it will never be called because tryPerformLTKcopy returns true and is not overwritten, so a MoveFilesAndFoldersOpertation is turned into a CopyFilesAndFoldersOperation.

Originally posted by @hd42 in 0aaee1a

[iloveeclipse]

Can you elaborate more: does it always affect generic move operation, or some preconditions needed, and what is the user visible problem?
I assume this is regression in 4.41?

[hd42]

Yes this is a regression in 4.41.
I couldn't reproduce it in the UI (maybe the tryPerformLTKcopy is returning false there?), but one of our integration tests failed because the supposedly moved files were still there.
From only looking at the code I can understand that behavior. The MoveFilesAndFoldersOperation overwrites getUndoableCopyOrMoveOperation but not tryPerformLTKcopy, so a call to MoveFilesAndFoldersOperation#copyResourcesInCurrentThread(IResource[], IContainer, IProgressMonitor) that succeeds with tryPerformLTKcopy creates a copy instead of moving.

[merks]

@hd42

Do you think you could propose a PR to fix the problem?

[hd42]

The only fix I would feel confident in is to revert the change, as I don't see a corresponding move method in the LTKLauncher. So I fear someone with more knowledge of LTK will have to look at this.

[Felix-Schmid]

I have provided a PR that should mitigate this issue: #4254
It restores the previous behavior for MoveFilesAndFoldersOpertation while still keeping the LTK copy for CopyFilesAndFoldersOperation.

[hd42]

A pragmatic solution that fixes the regression.
A further refactoring and LTK-based move implementation might be better in the future, but I assume it won't be a priority.

[iloveeclipse]

Unfortunately I'm on vacation and have no access to pc. @trancexpress , could you please check the proposed PR and whether the issue can surface somehow outside tests? It seem to be severe enough to fix it in RC1

[trancexpress]

could you please check the proposed PR

Seems fine, would be nice to also add a test. But if we are in a hurry that can be done later on. Do you need me to also merge the PR?

whether the issue can surface somehow outside tests?

I don't have much time to check this; after trying for some time and reading code, I didn't find a way.

I tried moving files and folders in the Project Explorer (the actions which call the affected code seem to be for that). Code which creates MoveFilesAndFoldersOperation does so to do validations, actual copying/moving is done by other operations. I.e. I can't get calls to CopyFilesAndFoldersOperation.performCopy() for a MoveFilesAndFoldersOperation object (MoveFilesAndFoldersOperation extends CopyFilesAndFoldersOperation).

Generally the move is done with a refactoring operation. I can get a call to CopyFilesAndFoldersOperation.performCopy() itself on a CopyFilesAndFoldersOperation object in:

eclipse.platform.ui/bundles/org.eclipse.ui.navigator.resources/src/org/eclipse/ui/navigator/resources/ResourceDropAdapterAssistant.java

Line 424 in 889d5d5

private IStatus performResourceMove(CommonDropAdapter dropAdapter,

[..]
		if (shouldLinkAutomatically) {
			CopyFilesAndFoldersOperation operation = new CopyFilesAndFoldersOperation(getShell());
			operation.setCreateLinks(true);
			operation.copyResources(sources, target);
[..]

This is when a file/folder is dropped into a virtual folder (https://help.eclipse.org/latest/index.jsp?topic=%2Forg.eclipse.platform.doc.user%2Fconcepts%2Fvirtualfolders.htm). But that operation creates copies.

Maybe a way exists to run into a problem, but its not everyday work.

This seems like a potential way to run into the regression, but I have no idea how to trigger a call:

eclipse.platform.ui/bundles/org.eclipse.ui.navigator.resources/src/org/eclipse/ui/navigator/resources/ResourceDropAdapterAssistant.java

Lines 281 to 297 in 889d5d5

	public IStatus handlePluginTransferDrop(IStructuredSelection aDragSelection, Object aDropTarget) {
	IContainer target = getActualTarget((IResource) aDropTarget);
	IResource[] resources = getSelectedResources(aDragSelection);
	MoveFilesAndFoldersOperation operation = new MoveFilesAndFoldersOperation(
	getShell());
	operation.copyResources(resources, target);
	if (target != null && target.isAccessible()) {
	try {
	target.refreshLocal(IResource.DEPTH_ONE, null);
	} catch (CoreException e) {
	}
	}
	return Status.OK_STATUS;
	}

(see section "Plugin drop handling" of https://www.eclipse.org/articles/Article-Workbench-DND/drag_drop.html)

[merks]

I had a look at the code and it looks safe to me. Given this is a regression, given @hd42 confirmed it fixes the regression, and given we want to move forward, my sense is we should merge this.

@akurtakov

Do you have an opinion?

[akurtakov]

It sounds reasonable to apply this fix .

[merks]

@iloveeclipse @trancexpress

If you guys are okay to move forward, I would merge this...

[trancexpress]

Looks fine from my POV.

[merks]

Given @iloveeclipse should be enjoying his well-deserved vacation, rather than following PRs, I will merge this the PR.