Skip to content
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 'Find Next' action at using Enter #7467

Closed
wants to merge 1 commit into from
Closed

Conversation

RomanNikitenko
Copy link
Contributor

What it does

Fixes #7460

I investigated the issue and found that an editor gets focus before execution a command.
It leads to the bug described in the issue.

How to test

Please use steps to reproduce from #7460

Review checklist

Reminder for reviewers

Signed-off-by: Roman Nikitenko rnikiten@redhat.com

Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
@RomanNikitenko
Copy link
Contributor Author

RomanNikitenko commented Mar 31, 2020

To be honest, I don't know why editor.focus() was added before execution a command.

@akosyakov
could you take a look
is it really required or it was a workaround for some command.

I tested these changes using tests from #7265.
They passed successfully for me.

It confuses me that editor.focus() lives here about 3 years, so maybe I should investigate more deeply how it worked before Monaco upgrade - maybe something was changed on VS Code side...

@kittaakos
Copy link
Contributor

@RomanNikitenko, I am currently refactoring the monaco-command-registry, monaco-commands, and monaco-editor-provider modules. Can we wait a day or so with this PR? It's not about any possible conflicts, but I might end up removing the execute method from the Monaco command registry.

CC: @akosyakov

@RomanNikitenko
Copy link
Contributor Author

@kittaakos

Can we wait a day or so with this PR?

It's OK for me, in the meantime I can investigate how it worked before upgrade Monaco and what was changed on VS Code side

@kittaakos
Copy link
Contributor

I can investigate how it worked before upgrade Monaco and what was changed on VS Code side

That would be a great help, also when it comes to the verification of #6428.

@akosyakov akosyakov added the monaco issues related to monaco label Apr 1, 2020
@akosyakov
Copy link
Member

@RomanNikitenko strange that it worked before with focus.

@RomanNikitenko
Copy link
Contributor Author

Closing in favor of #7481

@RomanNikitenko RomanNikitenko deleted the fixSearchNext branch April 6, 2020 12:47
@RomanNikitenko
Copy link
Contributor Author

strange that it worked before with focus

Before upgrade Monaco Find Next action was executed without using the method, so an editor did not get a focus at execution a command.
I checked it for 739522d:

find_next_before

For current master branch:

find_next_master

Something similar you can see in the second part of the comment: #7481 (review)

@akosyakov
Copy link
Member

@RomanNikitenko thanks for checking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monaco issues related to monaco
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search behaves differently than it used to (and compared to VS code)
3 participants