Edge Browser Key Event Async Handling #2860
Conversation
The method Widget:filters is made public in this commit as it is used by many children classes implementing it and having it package protected prevents the children outside the package to use it.
amartya4256
changed the title
| } | ||
|
|
||
| boolean filters (int eventType) { | ||
| public boolean filters (int eventType) { |
There was a problem hiding this comment.
I am not convinced that this should become public at all. It would help if you add javadoc (mandatory if its to become public!) that explains why/what/how/when this should be used.
There was a problem hiding this comment.
Is it even necessary to do this?
- Couldn't you also just create a package-protected method in
Browserthat is accessed byEdgeto get the information, or ... - couldn't you even just use
browser.getDisplay().filters...here?
Note that I have not tried to understand the overall intent of the PR yet, so I cannot judge whether the proposal as a whole makes sense. The comment is just about making this method public.
There was a problem hiding this comment.
Display:filters is also package protected. At first I thought we could make this public but then I saw all the widget children classes which use filters they use Widget#filters so for a better convenience, I made that public instead.
There was a problem hiding this comment.
I made that public instead.
Note, if this is supposed to be public API, it should provide also a proper javadoc clearly explaining the API contract (beside the usual rules that since tag is needed, bundle version bump etc).
There was a problem hiding this comment.
Just so it's 100% clear - no version bump should be needed as minor version is already bumped for this release via #2824
There was a problem hiding this comment.
I added the javadoc. Does that look alright or should it be different?
There was a problem hiding this comment.
Does that look alright or should it be different?
Just read the method name and try to imagine you would as a user need to use it.
Here expected reaction looking at the API proposed:
"filters"... What is that?
Filter what?
What kind of "eventType" should be given as argument?
Why filters says anything about listeners: "if there exists a listener"?
Where is there? On the current widget? All widgets in the hierarchy? Display?
If you would try to answer to the questions above, you would realize that this method helps to check whether there are filters registered on the display for given event type.
So I believe if it should be API, it should be named "hasFilters(eventType)" and located at Display, next to addFilter() and "removeFilter()". And of course javadoc should match the javadoc provided by two other methods.
There was a problem hiding this comment.
Naming-wise, I think its fine. filters here is used as a verb, as in, if it filters the event. I think, that was the intention of naming it like that here. So convenience wise the methods mean, add a filter to display, remove a filter from display and if display filters an event. hasFilter is also correct but so is filter. But if we name it hasFilter, either we need to provide a new method which is a copy of filters or we need to rename filters to hasFilter for which we need to adapt all its consumers. I think we can go with the name filter and provide more in-depth explanation in the javadoc just the way it is done is addFilter.
I'll re-adapt the javadoc and push.
| }); | ||
|
|
||
| boolean hasListener = browser.getListeners(keyEvent.type).length != 0 || browser.filters(keyEvent.type); | ||
| System.out.println(hasListener); |
There was a problem hiding this comment.
this is not a draft PR, so please remove all debug code.
There was a problem hiding this comment.
Ahh thanks, missed that one out.
amartya4256
force-pushed
the
amartya4256/edge_key_event
branch
2 times, most recently
from
1079f40 to
81d9314
Compare
Edge:handleAcceleratorKeyPressed must handle Browser:sendKeyEvent asynchronously as KeyEvent's Listeners can cause deadlock. Hence, the Handled of this event is set if there exists a listener for the event or a Display filter.
amartya4256
force-pushed
the
amartya4256/edge_key_event
branch
from
81d9314 to
a7c24df
Compare
4 participants
This PR does the following:
args.set_handledin the method must set the value to true only if the shortcut is handled by SWT, i.e., if there's a listener and handles the key combo. Since we cannot check that and set the value while executing asyncly, we needed to find another way to handle it.Solution: set the Handled to true when there's a listener for key event.
Drawbacks: Of course that means that it will set Handled to true for every Key Event regardless of whatever combination of keys pressed.