[Embedder] Wire view focus event and focus request#163930
[Embedder] Wire view focus event and focus request#163930knopp merged 10 commits intoflutter:masterfrom
Conversation
github-actions
bot
added
engine
knopp
force-pushed
the
view_focus_event
branch
from
5ddec48 to
4507bbf
Compare
knopp
changed the title
knopp
force-pushed
the
view_focus_event
branch
from
4507bbf to
d1aa858
Compare
github-actions
bot
added
platform-ios
knopp
force-pushed
the
view_focus_event
branch
from
d1aa858 to
cdb9c1b
Compare
github-actions
bot
added
the
a: accessibility
knopp
force-pushed
the
view_focus_event
branch
3 times, most recently
from
bea686b to
cbbf1db
Compare
knopp
force-pushed
the
view_focus_event
branch
from
cbbf1db to
0a94164
Compare
| /// @brief Notify the delegate that platform view focus state has changed. | ||
| /// | ||
| /// @param[in] event The focus event describing the change. | ||
| virtual void OnPlatformViewSendViewFocusEvent( |
There was a problem hiding this comment.
Should we just default this OOL to be a no-op? You won't have to patch the mocks.
There was a problem hiding this comment.
No other delegate method has default implementation. If the inconsistency is not an issue we can do it.
There was a problem hiding this comment.
Yeah, I don't think we had that many mocks. This is fine. But we can probably also clean up in a later patch.
mattkae
left a comment
mattkae
left a comment
There was a problem hiding this comment.
A few things, but this mostly look sensible to me 🎉
| // Focus state of a View. | ||
| // Must match ViewFocusState in ui/platform_dispatcher.dart. | ||
| enum class ViewFocusState : int64_t { | ||
| kUnfocused = 0, |
There was a problem hiding this comment.
Nit: = 0 is redundant.
There was a problem hiding this comment.
I like to have explicit value when mapping to values from another language. It is also used in key_data.h and possibly other places so I'd leave it there for consistency.
|
The embedder API and the engine updates LGTM. But it looks like @mattkae has some feedback on the API. I'll defer to them for the approval. Thanks! |
knopp
force-pushed
the
view_focus_event
branch
from
80739ed to
b8f4a03
Compare
| void PlatformConfigurationNativeApi::RequestViewFocusChange(int64_t view_id, | ||
| int64_t state, | ||
| int64_t direction) { | ||
| ViewFocusChangeRequest request{view_id, // |
There was a problem hiding this comment.
Missing comment or should the // be removed?
There was a problem hiding this comment.
The comment is there to force line break. Otherwise clang format puts first two argument in same line which doesn't look very readable. It's used in other parts of the engine as well.
|
Hey, shouldn't the enum members be namespaced, like the rest of the identifiers? I actually got a build breakage because of this, because I use |
7 participants
This wires
PlatformDispatcher.onViewFocusChangeandPlatformDispatches.requestViewFocusChangethrough embedder API.If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.