Process KeyBinding Command Asyncly for Browser #3614
Conversation
There was a problem hiding this comment.
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
...les/org.eclipse.e4.ui.bindings/src/org/eclipse/e4/ui/bindings/keys/KeyBindingDispatcher.java
Outdated
Show resolved
Hide resolved
amartya4256
force-pushed
the
amartya4256/process_command_asyncly_for_browser
branch
4 times, most recently
from
28ef122 to
2172c4d
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
HeikoKlare
force-pushed
the
amartya4256/process_command_asyncly_for_browser
branch
from
2172c4d to
0e6c654
Compare
| Object obj = HandlerServiceImpl.lookUpHandler(context, command.getId()); | ||
| if (obj != null) { | ||
| if (obj instanceof IHandler handler) { | ||
| commandHandled = command.isEnabled() && handler.isHandled(); |
There was a problem hiding this comment.
Why is the command.isEnabled() check added here?
There was a problem hiding this comment.
if the command is not enabled, the code simply registers an execption and proceeds. Later this leads to commandHandled being set to false. Note that this needed to decide if the current handler eats the key. If the eatKey is false then the next handler in the notifyListener handles this event. e.g. CTRL + A can have multiple handler and if the first handler is not enabled and the second one should perform the command. The first one should set the commandHandled to false and not eatKey so that it would be propagated to the second handler. Since we can check this proactively before handlerService#executeHandler, we leverage this to already check if it can execute. Similar check is also performed inside Command#execute.
There was a problem hiding this comment.
Great, thank you for the detailed explanation!
2 participants
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.