-
Notifications
You must be signed in to change notification settings - Fork 34.7k
fix: #75693 #75695
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: #75693 #75695
Conversation
Thanks for the PR, however this introduces some other issues. Here's one problem with your approach: fyi @jeanp413 |
The situation seems little bit more complicated than I thought. Sometimes(and always when changing the filename of children of root) IMHO, whether my approaching is good or not, having a |
Seems to work good in all cases. I will remove the disablment of the refresh and collapse actions since this is no longer needed |
My pleasure to giving a consent to merging such a code. |
@orange4glace unfortunetely I had to revert your PR becuase it caused an issue on linux. When the user clicks outside of the input box the success shuold be false and there should be no rename. |
@isidorn You mean on linux, there's no renaming when input is going blur While the intended result is being renamed? |
@orange4glace on linux just do that and you will see an exception in the console. |
@isidorn No apology is needed :) |
You are correct we do behave like that in stable. fyi @joaomoreno |
@isidorn @orange4glace I think a better fix would be changing this line
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? |
Also @orange4glace I tried your solution but it doesn't fix #72626. I can reproduce it at least on linux (ubuntu). |
@jeanp413 That piece of code actually fixes the issue that I said! That's cool. 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 |
I manage to fix #75624 with the following snippet in 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. |
Thank you very much for looking deeper into this.
@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 |
I can not repro on mac due to that I believe this is a dup of #75825 |
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. |
I can not repro any of that on Ubuntu weirdly. |
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 likeblur
orEnter 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 whenblur
event is called and thatblur
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 evenblur
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 ortimeout
forblur
event, giving 100mstimeout
forfocus
after the creation of the input seems to solve the overall problems.