Skip to content

Conversation

@amartya4256
Copy link
Contributor

This PR adapts how KeyBindingDispatcher processed Key events in case of Browser. If the trigger of the event is a browser, it processes the command asynchronously to avoid any possible deadlocks.

Contributes to #3104

Problem Explained
KeyBindings are registered globally with the Display. Inside the browser when a key combination is pressed which might open up another browser and use it synchronously, the browser needs to wait for the callback to finish and since it is not possible in case of Edge browser, it goes in a deadlock.

To achieve this, we must process the commands asynchronously. However, we need to tell the browser if the event eats the key event (sets event.doit = false) as the key is handled by SWT and not the browser natively. Hence, we need to proactively check if a command for the key event is going to be executed. If yes, it eats the key otherwise the browser can execute native action on the key event.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 29, 2025

Test Results

 3 011 files   - 4   3 011 suites   - 4   2h 17m 55s ⏱️ + 7m 36s
 8 258 tests ±0   8 007 ✅  -  3  248 💤 ±0  2 ❌ +2  1 🔥 +1 
23 592 runs   - 6  22 795 ✅  - 12  790 💤  - 1  4 ❌ +4  3 🔥 +3 

For more details on these failures and errors, see this check.

Results for commit 624385c. ± Comparison against base commit 0a6e39e.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a first glance, this looks like a reasonable workaround according to the proposal in #3104 (comment).

One concern regarding behavior is the clearance of the keyAssistDialog state. And note that a bunch of related tests is failing now.
My other current comments are rather regarding style.

Comment on lines 325 to 330
* Now that the command has executed (and had the opportunity to use the
* remembered state of the dialog), it is safe to delete that information.
*/
if (keyAssistDialog != null) {
keyAssistDialog.clearRememberedState();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may clear the state before the command has been executed (in case the command is executed asynchronously). Is that intended? I am not sure what is meant by

had the oppportunity to use the remembered state of the dialog

and if clearing that state potentially too early might be wrong now.

Doing this inside the handleCommandExecution would also be wrong, as it can take arbitrary long to execute the command via asyncExec, so that a state may be cleared that belongs to a later multi-key assistance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public void clearRememberedState() {
		previousWidth = NO_REMEMBERED_WIDTH;
	}

This just resets the previousRememberedState and since it can execute asynchronously, we want it to always have the previous width. Even though a task executed before it, if the width was changed and memorized, we will use that and reset the width asynchronously.

Hence, it is okay to move this block inside the method handleCommandExecution

Comment on lines 334 to 335
private void handleCommandExecution(final ParameterizedCommand parameterizedCommand,
final EHandlerService handlerService, Event trigger, Object obj) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are two out of three parameters final?
And couldn't you pass a commandExecutor that incorporates the handlerService and trigger/staticContext to avoid so many parameters? I.e., passing a lambda command -> handlerService.executeHandler(command, staticContext) instead of handlerService and trigger separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to dispose the context after the command has been processed. That's why I passed the event and created the context at the time of execution. Can also pass staticContext instead of trigger.

This commit adapts how KeyBindingDispatcher processed Key events in case
of Browser. If the trigger of the event is a browser, it processes the
command asynchronously to avoid any possible deadlocks.

Contributes to eclipse-platform#3104
@amartya4256 amartya4256 force-pushed the amartya4256/process_command_asyncly_for_browser branch from 68d169d to 624385c Compare December 30, 2025 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Freeze on Edge instantiation via BrowserViewer

2 participants