Fix breakpoint context menu behaviour

[thegecko]

Signed-off-by: thegecko [email protected]

What it does

This PR fixes #6237 by ensuring the editor window is correctly focussed and the current position is stored while managing breakpoints via the context menu.

Specifically:

• focus the current editor widget before showing the context menu. This ensures the current and active editor is correctly set in the shell for when the context menu is shown.

• Remove the onhide behaviour of the context menu which resets the current position. This was causing a race condition where the position was trying to be read by breakpoint management functions and it was no longer set (causing the breakpoint to appear on the first or last line).
  @akosyakov Was there any reason for the position to be reset when closing the menu? I didn't see any effects with stopping this.

• BONUS: I noticed the onhide event for the electron menus was being fired immediately, This is now fired when the menu is closed as expected.

How to test

Please refer to issue #6237 for reproduction steps.

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

[thegecko]

it does not add a new breakpoint, but remove the breakpoint added via the context menu

Hmm, in electron this works as expected and I don't see this behaviour

[thegecko]

@akosyakov It works fine because toggling the breakpoint without the context menu explicitly sets the position:

https://github.com/eclipse-theia/theia/blob/master/packages/debug/src/browser/editor/debug-editor-model.ts#L293

All other uses of toggle are via the context-menu and require the position to be set. I'll update the PR to tidy this slightly.

[akosyakov]

sorry, I don't get it, I meant this method returns the bogus position after closing the context menu:

theia/packages/debug/src/browser/editor/debug-editor-model.ts

Lines 243 to 246 in aa36ae6

	protected _position: monaco.Position | undefined;
	get position(): monaco.Position {
	return this._position || this.editor.getControl().getPosition()!;
	}

testing now again

[akosyakov]

It does not work for me, please see the screencast:

Toggle is using the wrong line after using the context menu.

[thegecko]

Thanks for the update, I wasn't aware toggle appeared in the main menu.

I assume you can recreate the original problem?

[akosyakov]

I assume you can recreate the original problem?

Sorry, i did no come to it, i've just looked at the code and though about the use case with toggle breakpoints keybinding.

[thegecko]

Thanks for your feedback @akosyakov

PR has been updated to only use the position of the context menu for commands in the context menu. This fixes the original issue and ensures the debug commands also continue to work as expected.

[akosyakov]

I'm probably missing something: Why do we need to change APIs, won't it be enough to clean up the context position when the menu is closed as it was before:

theia/packages/debug/src/browser/editor/debug-editor-model.ts

Line 290 in 57be198

setTimeout(() => this._position = undefined)

Got it.

[akosyakov]

I wonder whether we can pass the current position as an arg to the context menu instead of remembering it: https://github.com/ARMmbed/theia/blob/9cb7a32db69f10e2d15810e0ce27248f96f2171a/packages/core/src/browser/context-menu-renderer.ts#L37

Editor context menu commands then can use it, but it will remove a need to keep the context position which is potentially can lead to bugs as #6480 (comment)

[thegecko]

I'll take a look, I was struggling to fix this in a clean way

[thegecko]

Thanks @akosyakov . The PR has been updated to use the args sent to the context menu render.
This creates a much neater solution, please take a look.

[akosyakov]

It looks cleaner, but I don't think that the debug editor service should care whether it is called from context menu or not. It should provide APIs to manipulate breakpoints. It seems that we also lose some APIs like accessing a breakpoint for the currently selected line. It would be good to preserve them to avoid breaking downstream clients.

[akosyakov]

Tested that I can add/remove/edit breakpoints from context menus, with keybindings and from the breakpoints view for editor and editor preview widgets. It worked nicely for me.

[thegecko]

@akosyakov Updated to reflect your change requests. Have used a local isPosition() function in lieu of one available on monaco.Position.

Happy to merge?

[akosyakov]

@thegecko yes, i was happy to merge before as well :)