-
Notifications
You must be signed in to change notification settings - Fork 229
Process KeyBinding Command Asyncly for Browser #3614
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
base: master
Are you sure you want to change the base?
Process KeyBinding Command Asyncly for Browser #3614
Conversation
Test Results 3 015 files ±0 3 015 suites ±0 2h 6m 33s ⏱️ -10s For more details on these errors, see this check. Results for commit d3fe6b9. ± Comparison against base commit 7445acd. ♻️ This comment has been updated with latest results. |
There was a problem hiding this 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.
...les/org.eclipse.e4.ui.bindings/src/org/eclipse/e4/ui/bindings/keys/KeyBindingDispatcher.java
Outdated
Show resolved
Hide resolved
...les/org.eclipse.e4.ui.bindings/src/org/eclipse/e4/ui/bindings/keys/KeyBindingDispatcher.java
Outdated
Show resolved
Hide resolved
...les/org.eclipse.e4.ui.bindings/src/org/eclipse/e4/ui/bindings/keys/KeyBindingDispatcher.java
Outdated
Show resolved
Hide resolved
| private void handleCommandExecution(final ParameterizedCommand parameterizedCommand, | ||
| final EHandlerService handlerService, Event trigger, Object obj) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Then it's probably best to keep the trigger parameter as it's otherwise confusing that you pass over the lifecycle handling for staticContext from the caller to the callee. Instead, the caller would have to dispose it.
68d169d to
624385c
Compare
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
624385c to
d3fe6b9
Compare
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.