fix: #75693

[orange4glace]

This is a PR for #75693

This problem occurs since after the commitment of the #72627.
Since there' s a finishEditing flag, only the UI event like blur or Enter Key ACTUALLY finish the editing mode and make File Explorer as active again.
When refreshing File Explorer by external events, like file changes by the asynchronous terminal command, never makes finishEditing as true so the onFinish will be never called.

The reason why finishEditing exists is that input box will be disposed when blur event is called and that blur event is called immediately after the creation of the input box for some reason.
The very first approach to resolve this problem was giving the timeout for 100ms to prevent disposing the input even blur is called.
But that gives another bug so finishEditing flag was introduced few days ago.

And now it causes another bug which is being stated here so rather than giving a finishEditing flag or timeout for blur event, giving 100ms timeout for focus after the creation of the input seems to solve the overall problems.

[msftclas]

All CLA requirements met.

[isidorn]

Thanks for the PR, however this introduces some other issues.
I hate that input box code with a passion, and the current solution has the least flows.

Here's one problem with your approach:
Have a folder collapsed (never expanded). Right click -> new file. Exception in console.

fyi @jeanp413

[orange4glace]

The situation seems little bit more complicated than I thought.
This commit might fix the issue.

Sometimes(and always when changing the filename of children of root) renderElement is called more than once.
In that situation, two input boxes are created and the first one is destroyed before setTimeout is called so the exception happens.
To prevent this, before focusing the input, it checks whether the input is disposed or not by checking !inputBox.inputElement.
In addition, since dispose event of the input is fired immediately when two input boxes are created, I made a flag isFinishableDisposeEvent to decide whether the dispose event is actually valid to change editable state or not.

IMHO, whether my approaching is good or not, having a finishEditing flag seems not to be a gentle one. Having finishEditing flag fundamentally prevents finishing editing if there's no user actions like blur or ENTER. But the File Explorer can be interrupted by external events like the 'terminal situation' like I said, and it can not handle that situation properly.

[isidorn]

Seems to work good in all cases.
It is quite delicate (ugly) code. So thanks a lot of understanding it and improving it it seems.
Thank you 🙏

I will remove the disablment of the refresh and collapse actions since this is no longer needed

[orange4glace]

My pleasure to giving a consent to merging such a code.

[isidorn]

@orange4glace unfortunetely I had to revert your PR becuase it caused an issue on linux.
The issue being
Right click on a file -> rename. Click anywhere else. We think the sucess is true and the fileService tries to rename the file, while it shuold not.
This only throws on linux for some reason (probably implementation of the file service due to case sensitivity).

When the user clicks outside of the input box the success shuold be false and there should be no rename.

[orange4glace]

@isidorn You mean on linux, there's no renaming when input is going blur While the intended result is being renamed?

[isidorn]

@orange4glace on linux just do that and you will see an exception in the console.
When the input box is going blur there should be no rename on any OS. On linux this throws an exception so that is why we caught it there. I appologise that I did not notice this when doing an inital review of your PR.

[orange4glace]

@isidorn No apology is needed :)
But I'm wondering that even current stable release version (which can be downloaded from https://code.visualstudio.com/) has a such behavior that blur input box actually do rename it.

(with official stable version, 1.35.1)

[isidorn]

You are correct we do behave like that in stable.
Now I checked and the natvie mac finder and the native windows explorer and they do the same - they accept on blur.
So I was wrong in my previous post. This should be a success.
However it seems that just some check is missing if the name did not change that this is a no-op. Since the fileService throws on linux

fyi @joaomoreno

[jeanp413]

@isidorn @orange4glace I think a better fix would be changing this line

vscode/src/vs/workbench/contrib/files/browser/views/explorerView.ts

Line 173 in 3c69838

this._register(this.explorerService.onDidChangeItem(e => this.refresh(e.recursive, e.item)));

to something like this

this._register(this.explorerService.onDidChangeItem(e => {
    if(this.explorerService.isEditable(undefined)){
        this.tree.domFocus();
    }
    this.refresh(e.recursive, e.item)
}));

what do you think?

[jeanp413]

Also @orange4glace I tried your solution but it doesn't fix #72626. I can reproduce it at least on linux (ubuntu).
And now there is this issue #75624 that I think my changes introduced and seems more difficult to solve with my approach but your PR fixes it.

[orange4glace]

@jeanp413 That piece of code actually fixes the issue that I said! That's cool.
Although as you said, it seems cannot solve the issue #75624 though I have no idea how to reproduce that problem with a stable version. It's kind of weird since that problem seems to be related with blur event but your code (current stable version) handles it well. If I can reproduce it and test it, I could tell with more confidence.

For a dangling problem, I think this commit would fix it. Since PR was closed once, it seems I have to create another PR to apply the commit. If you guys are like, I'll make another PR for it.

@isidorn The linux problem, I've tested on my VM with Ubuntu 16.04 and getting error Unable to move/copy when source path is equal or parent of target path: Error: Unable to move/copy when source path is equal or parent of target path
It seems this problem was in place for a long time. I've tested with the reverted version and same error occurs.
We might need a guard to check whether the filename is same or not.

[jeanp413]

I manage to fix #75624 with the following snippet in explorerView.ts although it modifies too many files, basically checks if the element being edited is no longer visible and forces to cancel the editing

this._register(this.tree.onDidScroll(e => {
	let editable = this.explorerService.getEditable(); // This method is new, needs to modify IExplorerService
	if (editable && this.tree.getRelativeTop(editable.stat) === undefined) {
		editable.data.onFinish('', false);
	}
}));

Also as orange4glace said orange4glace@9c29fb8 fixes #72626

So now both approaches fix all the issues and I think it boils down to chose the one with less drawbacks.

[isidorn]

Thank you very much for looking deeper into this.

1. The move/copy Linux error is due to the fileService. I have created this issue File service: unable to move/copy same path #75825 to track that, and because we were behaving like this before I have reaplied this PR
2. The danling problem: @orange4glace please create a new PR with just that change
3. There might be more problems with the rename file in explroer, scroll a lot -> input box remains there and tree looks broken. I will try to find a repro case for this and investigate more.

@jeanp413 I decided to go with the @orange4glace since the changes are directly in the viewer the top most layer, and I prefer to put workarounds as close as possible to the actual problem. Also this solution came in first.

fyi @joaomoreno

[jeanp413]

@isidorn I think we still need to disable the new file and new folder actions (from db17c23) while editing an item in the explorer. I'm having this bug.

[isidorn]

I can not repro on mac due to that I believe this is a dup of #75825
Do you get some errors in the F1 > developer console

[jeanp413]

I can repro even if I change the filename (to avoid #75825 so I get no errors in the developer tools console) before clicking the new file/folder button.

[isidorn]

I can not repro any of that on Ubuntu weirdly.
@jeanp413 Can you maybe open a new issue for that, so we do not lose it. Thanks a lot!