Fix for DnD files to main area #8739

[balajiv113]

Signed-off-by: Balaji V [email protected]

What it does

Fixes #8739

The cause of the issue is FileNavigatorCommands.OPEN command opens the file based on tree widget selections, before dropping in editor the selection was not updated correctly in navigator-widget.

The below changes updates the tree selection and then fires the FileNavigatorCommands.OPEN command

How to test

1. Open any folder with some files
2. Select file & drag and drop to main area

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines
• Tried with multiple selection from different folders

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[colin-grant-work]

This fix looks reasonable, but it may not be addressing the root problem. It seems like the real issue is that selection might be expected to happen on mouse down, or if not that, then at least onDragStart. A more comprehensive fix for this and any similar issues might be to toggle the selection state of the dragged node in this function:

theia/packages/filesystem/src/browser/file-tree/file-tree-widget.tsx

Lines 106 to 129 in 39887bf

	protected handleDragStartEvent(node: TreeNode, event: React.DragEvent): void {
	event.stopPropagation();
	let selectedNodes;
	if (this.model.selectedNodes.find(selected => TreeNode.equals(selected, node))) {
	selectedNodes = [...this.model.selectedNodes];
	} else {
	selectedNodes = [node];
	}
	this.setSelectedTreeNodesAsData(event.dataTransfer, node, selectedNodes);
	if (event.dataTransfer) {
	let label: string;
	if (selectedNodes.length === 1) {
	label = this.toNodeName(node);
	} else {
	label = String(selectedNodes.length);
	}
	const dragImage = document.createElement('div');
	dragImage.className = 'theia-file-tree-drag-image';
	dragImage.textContent = label;
	document.body.appendChild(dragImage);
	event.dataTransfer.setDragImage(dragImage, -10, -10);
	setTimeout(() => document.body.removeChild(dragImage), 0);
	}
	}

[balajiv113]

@colin-grant-work - Totally agreed on your point, i have looked into fixing under handleDragStartEvent, But the problem i faced is

Even if we select nodes in handleDragStart, those will always will be reset by handleDragEnter below, The below code is for updating selection when dragging files and hovering to/inside a folder

theia/packages/filesystem/src/browser/file-tree/file-tree-widget.tsx

Lines 131 to 139 in c79e9d4

	protected handleDragEnterEvent(node: TreeNode | undefined, event: React.DragEvent): void {
	event.preventDefault();
	event.stopPropagation();
	this.toCancelNodeExpansion.dispose();
	const containing = DirNode.getContainingDir(node);
	if (!!containing && !containing.selected) {
	this.model.selectNode(containing);
	}
	}

I felt if we could have two kinds of node selection states like focused nodes and selected nodes (like in vscode), that would be better to address these issues.

[colin-grant-work]

Even if we select nodes in handleDragStart, those will always will be reset by handleDragEnter below, The below code is for updating selection when dragging files and hovering to/inside a folder

Hm... Yes, I see what you mean about the drag enter handler resetting the selection immediately. I agree that two focus/selected states might be helpful, and in fact there is code in various tree related files that anticipates a focused state.

theia/packages/core/src/browser/tree/tree-selection.ts

Lines 133 to 135 in 335273b

	export function hasFocus(node: TreeNode | undefined): boolean {
	return is(node) && node.focus === true;
	}

theia/packages/core/src/browser/tree/tree-widget.tsx

Lines 904 to 906 in 335273b

	if (SelectableTreeNode.hasFocus(node)) {
	classNames.push(FOCUS_CLASS);
	}

but that option isn't really exploited in any of the tree state management that I've seen, and implementing that is likely too much to ask for this PR 😄.

It looks like there's some confusion about how the FileNavigatorCommands.OPEN command is supposed to work. The old version of the code you've modified passes the URI to the command:

theia/packages/navigator/src/browser/navigator-widget.tsx

Line 112 in 335273b

treeNodes.filter(FileNode.is).forEach(treeNode => this.commandService.executeCommand(FileNavigatorCommands.OPEN.id, treeNode.uri));

But the current implementation of the command ignores anything passed and instead just refers to the selection state:

theia/packages/navigator/src/browser/navigator-contribution.ts

Lines 350 to 360 in 335273b

	registry.registerCommand(FileNavigatorCommands.OPEN, {
	isEnabled: () => this.getSelectedFileNodes().length > 0,
	isVisible: () => this.getSelectedFileNodes().length > 0,
	execute: () => {
	this.getSelectedFileNodes().forEach(async node => {
	const opener = await this.openerService.getOpener(node.uri);
	opener.open(node.uri);
	});
	}
	});
	}

I think that the reference to the selection state is to support invocation from a context menu.

In any case, it seems to me that the part of this that is least consistent with VSCode's behavior is the setting of the selection to the containing folder on any drag action - that simply doesn't happen in VSCode. I also don't see that that it's necessary, other than as a visual guide (which VSCode provides via focus, rather than selection), since on drop, the container is calculated anyway:

theia/packages/filesystem/src/browser/file-tree/file-tree-widget.tsx

Lines 162 to 183 in 335273b

	protected async handleDropEvent(node: TreeNode | undefined, event: React.DragEvent): Promise<void> {
	try {
	event.preventDefault();
	event.stopPropagation();
	event.dataTransfer.dropEffect = 'copy'; // Explicitly show this is a copy.
	const containing = this.getDropTargetDirNode(node);
	if (containing) {
	const resources = this.getSelectedTreeNodesFromData(event.dataTransfer);
	if (resources.length > 0) {
	for (const treeNode of resources) {
	await this.model.move(treeNode, containing);
	}
	} else {
	await this.uploadService.upload(containing.uri, { source: event.dataTransfer });
	}
	}
	} catch (e) {
	if (!isCancelled(e)) {
	console.error(e);
	}
	}
	}

I get reasonable behavior with the following changes:

• In handleDragStartEvent select the node if it is unselected.
• In handleDragEnterEvent just don't select the containing node.

The visual guide of the selected directory is lost, but the behavior is correct - and the visual guide should really be provided by other means. So the decision is whether to work around it for now, as you have in the code you've submitted, or fix it at the source, but leave work to be done on focus states in trees. If you're willing to take a stab at providing focus behavior in this case, that would be awesome, otherwise I'm probably inclined to accept your code - I don't foresee any negative side effects - and make a ticket specifically for focus behavior.

[balajiv113]

@colin-grant-work - As you were saying above using selection for the visual guide is what making the behaviour odd. I will try to take a look into providing a separate focus behaviour for tree in a separate PR. That way the visual guides and selection can be in consistent with the VSCode behaviour and these kind of issues can be fixed.

[colin-grant-work]

Sounds good. In that case, the only change I'd request here is that you use the SelectableTreeNode.isSelected function rather than checking directly for .selected on each node:

theia/packages/core/src/browser/tree/tree-selection.ts

Lines 129 to 131 in 7e8992b

	export function isSelected(node: TreeNode | undefined): node is SelectableTreeNode {
	return is(node) && node.selected;
	}

In the end, it's the same, but using the function ensures that if we later change how selection is handled (e.g. not with a field on the node itself), the change is carried through consistently.

[vince-fugnitto]

I've confirmed that it fixes the issue #8739 👍
I verified the following:

• drag-and-drop a file from the explorer to the main-area
• drag-and-drop multiple files from the explorer to the main-area

[vince-fugnitto]

@balajiv113 please resolve the conflicts, I do not think it'll be on time for today's release, but we can merge shortly afterwards.

[balajiv113]

@vince-fugnitto - Conflicts are resolved 👍

[vince-fugnitto]

@vince-fugnitto - Conflicts are resolved

@balajiv113 thank you for your contribution!