GH-6428: Refactored the built-in monaco commands. #7481

kittaakos · 2020-04-01T13:54:07Z

Signed-off-by: Akos Kitta [email protected]

What it does

Changes the way how we handle monaco commands, and hopefully providers a better UX for the Undo, Redo, and Select All commands. It is the fix for electron, as it

How to test

Test Plan:

Preconditions:
- If you are verify this in electron, you have to be avere of the non-dynamic nature of the application menu. ([electron][menu] Selection menu is missing from the electron app #6425).
- Run Reset Workbench Layout.
- Open the SIW widget; the input widget has the focus.
Steps:
- Type foo and press Ctrl/⌘+A (Select All). Assert: the input value is foo and is selected.
- Press Del. Assert: the input is empty.
- Press Ctrl/⌘+Z (Undo). Assert: the input value is foo and is selected.
- Press Ctrl/⌘+Shift+Z (Redo). Assert: the input is empty.
- Type bar and select the Selection > Select All menu item. Assert: the input value is bar and is selected.
- Press Del. Assert: the input is empty.
- Select the Edit > Undo menu item. Assert: the input is bar and is selected. (Note: there is a bug here. The SIW widget's input does not listen on value change and does not re-run the query.)
- Select the Edit > Redo menu item. Assert: the input is empty.

Preconditions:
- If you are verify this in electron, you have to be avere of the non-dynamic nature of the application menu. ([electron][menu] Selection menu is missing from the electron app #6425).
- Open theia as a workspace in Theia.
- Run Reset Workbench Layout.
- Open the SIW widget; the input widget has the focus, type baz.
- Open the os.ts module via the Command Palette. If it is opened in a preview mode, make sure the editor has the focus; the cursor blinks inside the editor.
Steps:
- Type foo somewhere in the editor and press Ctrl/⌘+Z (Undo). Assert: foo is not there.
- Press Ctrl/⌘+Shift+Z (Redo). Assert: foo is there.
- Press Ctrl/⌘+Z (Undo). Assert: foo is not there.
- Press Ctrl/⌘+A (Select All). Assert: the editor content is selected.
- Press Esc. Assert: the cursor blinks at the end of the file, nothing is selected.
- Type bar and select the Edit > Undo menu item. Assert: bar is not there.
- Select the Edit > Redo menu item. Assert: bar is in the editor.
- Set the focus (single-click) into the SIW's input.
- Press Ctrl/⌘+A (Select All). Assert: baz is selected.
- Make sure the cursor blinks once again in the editor and select the Selection > Select All menu item. Assert: the editor content is selected.
- Press Esc. Assert: the cursor blinks at the end of the file, nothing is selected.
- Right-click on isWindows in the editor and select Peek > Peek References from the context menu. Assert: embedded editor is open. The cursor blinks on isWindows in the main editor.
- Press Ctrl/⌘+A (Select All). Assert: the main editor content is selected.
- Make sure the cursor blinks this time in the embedded editor and press Ctrl/⌘+A (Select All). Assert: the embbedded editor content is selected. (Fixes [monaco] Select all does not work in inline, embedded editors #7483)

🎖Let's verify other issues:

Inside the os.ts module, press Ctrl/⌘+F, type is into the search widget. If you press Enter (multiple times) the selection jumps to the next is occurrence position in the editor. Bonus: it works inside the embedded editor too. (Fixes Search behaves differently than it used to (and compared to VS code) #7460)
If you are verifying the changes with the electron example (Fixes [electron] The Select All command executes incorrectly #6428), open the Command Palette and make sure you can do Undo, Redo, and Select All from the keyboard. (Fixes [electron] Ctrl/Cmd+A does not select the content of the quick open item #2756)
Close all editors and press Ctrl/⌘+F. Assert: no errors because of the missing handler for core.find command. (Fixes [core] Revisit core commands, such as Find, Replace, Select All, Undo, and Redo #7474)

TODOs:

Verify this. From time to time, I can see the Commiting pending changes before change accessor leaves due to read access. warning when running the electron example. Hmm, but the logger code was removed by the VS Code guys in January. Let's double-check it. Verified ✅ (microsoft/vscode@6beb757)

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

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

@akosyakov, @RomanNikitenko, here is an early preview of my changes. I am going to verify the behavior locally and write down the verification steps. Please take a look at my changes. Thank you!

packages/monaco/src/browser/monaco-command.ts

packages/core/src/browser/keybinding.ts

packages/monaco/src/browser/monaco-keybinding-contexts.ts

packages/monaco/src/browser/monaco-command.ts

packages/monaco/src/browser/monaco-keybinding-contexts.ts

packages/core/src/browser/common-frontend-contribution.ts

packages/core/src/common/command.ts

packages/monaco/src/browser/monaco-command.ts

akosyakov · 2020-04-02T07:01:58Z

@marechal-p Could you help with testing please too?

packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts

kittaakos · 2020-04-02T12:15:47Z

I did my best to address all remarks, the PR is ready for the review. @RomanNikitenko, @marechal-p can you please help with the verification? Thank you! 🙏

RomanNikitenko · 2020-04-02T12:47:38Z

@kittaakos
I can start to test the changes for browser.
Does it make sense?

or according to

It is the fix for electron

it's expected to test the changes for electron only

kittaakos · 2020-04-02T12:56:48Z

I can start to test the changes for browser.
Does it make sense?

Yes. Always 😊

it's expected to test the changes for electron only

Just go for the browser now.

RomanNikitenko · 2020-04-02T13:29:27Z

Sorry for misleading you, I have a meeting, can start testing in ~2 hours...

RomanNikitenko

@kittaakos
I'm testing browser side at the moment.

Looks like, for step:

Type bar and select the Selection > Select All menu item. Assert: the input value is bar and is selected.

I have the different behavior from VS Code:

So, it does nothing.
But it works from context menu.

For master I have another behavior, but is different from VS Code as well:

So it selects editor content, I think because:

the command is executed using this method
and
editor.focus() is present in master branch

Please let me know if my testing steps are wrong.

akosyakov

@kittaakos This PR is awesome! It is going to solve so many issues.

RomanNikitenko · 2020-04-06T09:39:44Z

@kittaakos
I'm testing the changes for browser.

About:

Right-click on isWindows in the editor and select Peek > Peek References from the context menu. Assert: embedded editor is open. The cursor blinks on isWindows in the main editor.
Press Ctrl/⌘+A (Select All). Assert: the main editor content is selected.

I'm not sure where the cursor is at this step
You can see the behavior here:

But I can apply Select All action for any area at this step, it fixes the master branch behavior and I have the same behavior for VS Code, so it looks good to me.

kittaakos · 2020-04-06T09:46:46Z

t: embedded editor is open. The cursor blinks on isWindows in the main editor.

Well, your cursor does not blink in the main editor, at least I cannot see it.

RomanNikitenko · 2020-04-06T09:50:56Z

@kittaakos
Regarding

Close all editors and press Ctrl/⌘+F. Assert: no errors because of the missing handler for core.find command. (Fixes #7474)

Looks like it's not fixed for me for browser side:

kittaakos · 2020-04-06T09:51:38Z

I'm not sure where the cursor is at this step

You can clearly see when you open the embedded editor, the selection is on the right-hand side, the selected item is blueish:

kittaakos · 2020-04-06T09:52:42Z

Looks like it's not fixed for me for browser side:

👍 Thanks for finding it. Checking...

kittaakos · 2020-04-06T10:06:20Z

Regarding

Close all editors and press Ctrl/⌘+F. Assert: no errors because of the missing handler for core.find command. (Fixes #7474)

Looks like it's not fixed for me for browser side:

Very useful findings. The following commands do not have a when clause.

root INFO actions.findWithSelection when null
logger-protocol.ts:112 root INFO actions.find when null

VS Code refs:

Closes #6428 Signed-off-by: Akos Kitta <[email protected]>

Signed-off-by: Akos Kitta <[email protected]>

kittaakos · 2020-04-06T10:12:37Z

Looks like it's not fixed for me for browser side:

Fixed now. It delegates to the browser's Find functionality, if there is no opened editor.

RomanNikitenko · 2020-04-06T11:27:47Z

Fixed now.

Testing...

RomanNikitenko

I tested for browser side.

Faced with the issue related to Ctrl+z withing testing the changes

Checked that master has the same problem, so it's not a regression.

I still can see Commiting pending changes before change accessor leaves due to read access.:

I can confirm that:

first two sets from the Test Plan work well for me
Find Next (Enter) and Find Previous (Shift +Enter) are fixed - thanks!
the problem was fixed - now It delegates to the browser's Find functionality

kittaakos · 2020-04-06T12:29:01Z

I still can see Commiting pending changes before change accessor leaves due to read access.:

We are not yet using that version. microsoft/vscode@6beb757
By the way, it happens with you open and close the Find widget in the editor.

Faced with the issue related to Ctrl+z withing testing the changes

Let me check this one too. Ohh, sorry 😕

kittaakos · 2020-04-06T12:30:20Z

Faced with the issue related to Ctrl+z withing testing the changes

@RomanNikitenko, did you see this when undoing in the editor? #7486

RomanNikitenko · 2020-04-06T12:37:32Z

@kittaakos
I think yes:

kittaakos · 2020-04-06T13:01:25Z

I leave this open till the end of the day if anyone wants to object. Thank you for the review @akosyakov and for the multiple verifications @RomanNikitenko 🙏

kittaakos force-pushed the core-commands branch from 04bef58 to 319cc15 Compare April 1, 2020 14:18