[linux] Adds raw key event handler #163
Conversation
| @@ -50,6 +51,7 @@ struct FlutterEmbedderState { | |||
| // deleted from the heap. | |||
| std::vector<flutter_desktop_embedding::KeyboardHookHandler *> | |||
| keyboard_hook_handlers; | |||
| std::unique_ptr<flutter_desktop_embedding::KeyEventHandler> key_event_handler; | |||
There was a problem hiding this comment.
Please add a quick comment explaining what this is, as you would with a class ivar.
library/linux/src/embedder.cc
Outdated
| @@ -275,6 +277,10 @@ GLFWwindow *CreateFlutterWindow(size_t initial_width, size_t initial_height, | |||
| FlutterEmbedderState *state = new FlutterEmbedderState(); | |||
| state->plugin_handler = std::make_unique<PluginHandler>(engine); | |||
| state->engine = engine; | |||
|
|
|||
| state->key_event_handler = std::make_unique<KeyEventHandler>(); | |||
| state->key_event_handler->SetBinaryMessenger(state->plugin_handler.get()); | |||
There was a problem hiding this comment.
Since you never need to change this, and the handler doesn't really do anything without it, strongly consider making this a constructor argument.
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| #include "library/linux/src/internal/key_event_handler.h" | ||
| #include "library/linux/src/internal/json_message_codec.h" |
There was a problem hiding this comment.
This header should be in its own section after the library headers, per Google style. Only the associated header (key_event_handler.h) goes above.
| break; | ||
| } | ||
| auto message = JsonMessageCodec::GetInstance().EncodeMessage(args); | ||
| messenger_->Send(channel_, message->data(), message->size()); |
There was a problem hiding this comment.
If you don't make messenger a constructor argument, you should check that it's not null before using it.
| args[kTypeKey] = kKeyUp; | ||
| break; | ||
| default: | ||
| break; |
There was a problem hiding this comment.
Wouldn't getting here be an error, or something that should trigger an early return? As written it would result in a key event being sent without a type.
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| #ifndef LIBRARY_LINUX_SRC_INTERNAL_KEY_EVENT_PLUGIN_H_ |
There was a problem hiding this comment.
s/PLUGIN/HANDLER in all of these guards.
| #include "library/linux/include/flutter_desktop_embedding/binary_messenger.h" | ||
| #include "library/linux/src/internal/keyboard_hook_handler.h" | ||
|
|
||
| #include <memory> |
There was a problem hiding this comment.
This goes before project headers.
| // TODO: This event is not supported by Flutter. Add once it's implemented. | ||
| static constexpr char kRepeat[] = "repeat"; | ||
|
|
||
| namespace flutter_desktop_embedding { |
There was a problem hiding this comment.
Be consistent about horizontal spacing; if the namespace is going to have a blank line at the end, put one at the start as well. (Same for header.)
| void SetBinaryMessenger(BinaryMessenger *messenger); | ||
|
|
||
| private: | ||
| const BinaryMessenger *messenger_; |
There was a problem hiding this comment.
If you don't make this constructor-initialized, you should initialize it to nullptr here.
| void KeyboardHook(GLFWwindow *window, int key, int scancode, int action, | ||
| int mods) override; | ||
|
|
||
| // KeyboardHookHandler. |
There was a problem hiding this comment.
Just one comment per superclass please; the idea is to group all the methods that are overridden from a specific interface:
// KeyboardHookHandler.
void KeyboardHook(...) override;
void CharHook(...) override;
[other methods here]
|
Comments addressed, PTAL! |
No description provided.