Skip to content

[linux] Adds raw key events plugin. #144

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions library/linux/src/embedder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include <flutter_embedder.h>

#include "library/linux/src/internal/key_event_plugin.h"
#include "library/linux/src/internal/keyboard_hook_handler.h"
#include "library/linux/src/internal/plugin_handler.h"
#include "library/linux/src/internal/text_input_plugin.h"
Expand Down Expand Up @@ -275,13 +276,17 @@ 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;

auto key_event_plugin = std::make_unique<KeyEventPlugin>();
state->keyboard_hook_handlers.push_back(key_event_plugin.get());
auto input_plugin = std::make_unique<TextInputPlugin>();
state->keyboard_hook_handlers.push_back(input_plugin.get());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: whitespace

glfwSetWindowUserPointer(window, state);

AddPlugin(window, std::move(key_event_plugin));
AddPlugin(window, std::move(input_plugin));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: whitespace

int width, height;
glfwGetWindowSize(window, &width, &height);
GLFWwindowSizeCallback(window, width, height);
Expand Down
60 changes: 60 additions & 0 deletions library/linux/src/internal/key_event_plugin.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// Copyright 2018 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// 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.
#include "library/linux/src/internal/key_event_plugin.h"
#include <flutter_embedder.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: whitespace before system includes

#include <iostream>

static constexpr char kChannelName[] = "flutter/keyevent";

static constexpr char kKeyCodeKey[] = "keyCode";
static constexpr char kKeyMapKey[] = "keymap";
static constexpr char kTypeKey[] = "type";

static constexpr char kAndroidKeyMap[] = "android";
static constexpr char kKeyUp[] = "keyup";
static constexpr char kKeyDown[] = "keydown";
// TODO: This event is not supported by Flutter. Add once it's implemented.
static constexpr char kRepeat[] = "repeat";

namespace flutter_desktop_embedding {
KeyEventPlugin::KeyEventPlugin() : JsonPlugin(kChannelName, false) {}

KeyEventPlugin::~KeyEventPlugin() {}

void KeyEventPlugin::CharHook(GLFWwindow *window, unsigned int code_point) {}

void KeyEventPlugin::KeyboardHook(GLFWwindow *window, int key, int scancode,
int action, int mods) {
Json::Value args;
args[kKeyCodeKey] = key;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a TODO comment to translate to a cross-platform key code system rather than just reflecting the platform-specific one up to the Dart layer?

args[kKeyMapKey] = kAndroidKeyMap;
switch (action) {
case GLFW_PRESS:
args[kTypeKey] = kKeyDown;
break;
case GLFW_RELEASE:
args[kTypeKey] = kKeyUp;
break;
default:
break;
}
/// Messages to flutter/keyevent channels have no method name.
InvokeMethod("", args);
}

void KeyEventPlugin::HandleJsonMethodCall(
const JsonMethodCall &method_call, std::unique_ptr<MethodResult> result) {
// There are no methods invoked from Flutter on this channel.
}
} // namespace flutter_desktop_embedding
39 changes: 39 additions & 0 deletions library/linux/src/internal/key_event_plugin.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright 2018 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// 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_
#define LIBRARY_LINUX_SRC_INTERNAL_kEY_EVENT_PLUGIN_H_

#include "library/linux/include/flutter_desktop_embedding/json_plugin.h"
#include "library/linux/src/internal/keyboard_hook_handler.h"

namespace flutter_desktop_embedding {
class KeyEventPlugin : public KeyboardHookHandler, public JsonPlugin {
public:
KeyEventPlugin();
virtual ~KeyEventPlugin();

// Plugin.
void HandleJsonMethodCall(const JsonMethodCall &method_call,
std::unique_ptr<MethodResult> result) override;

// KeyboardHookHandler.
void KeyboardHook(GLFWwindow *window, int key, int scancode, int action,
int mods) override;

// KeyboardHookHandler.
void CharHook(GLFWwindow *window, unsigned int code_point) override;
};
} // namespace flutter_desktop_embedding

#endif // LIBRARY_LINUX_SRC_INTERNAL_KEY_EVENT_PLUGIN_H_
4 changes: 4 additions & 0 deletions library/linux/src/json_method_codec.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ std::unique_ptr<MethodCall> JsonMethodCodec::DecodeMethodCallInternal(
std::unique_ptr<std::vector<uint8_t>> JsonMethodCodec::EncodeMethodCallInternal(
const MethodCall &method_call) const {
Json::Value message(Json::objectValue);
if (method_call.method_name().empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this code necessary now? Is this unrelated cleanup?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change shouldn't be made to MethodCodec.

The issue here is that flutter/keyevent is a MessageChannel, not a MethodChannel, on the Dart side. MethodChannel is essentially a layer over MessageChannel that establishes the name/argument structure. Making *MethodCodec allow non-Method-structured messages goes against that distinction.

The right fix, which would align with the overall work of #102 is to split out JsonMessageCodec from JsonMethodCodec, but that will also require building out support for Message-based plugins. The next step of plugin refactoring that I have planned would help lay the foundation for that.

Is support for raw key events on Linux time critical, or is this something we can revisit once I'm further along with #102? If the former, I think the best way forward in the short term to support the direction of #102 would be:

  • Split MessageCodec out from MethodCodec
  • Add a new class above Plugin (this would be temporary, since Plugin will be significantly changed or even eliminated as part of Make the plugin API structure match the existing Flutter plugin API #102 eventually anyway) that operates on generic data messages.
  • Make Plugin do the conversion from data to method call, then forward to the existing Plugin methods.
  • Change PluginHandler to take in that new superclass instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably wait. We were trying to get the overall raw key event feature to work in a short term time frame, but we can wait on the Linux implementation side.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's let this PR wait for a bit then. I'm hoping to do more work on plugin refactoring next week, and then we can re-evaluate.

If someone wants to do the MessageCodec/MethodCodec split in the mean time that would be useful parallel work that we'll need regardless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting SGTM.

return EncodeJsonObject(
*static_cast<const Json::Value *>(method_call.arguments()));
}
message[kMessageMethodKey] = method_call.method_name();
message[kMessageArgumentsKey] =
*static_cast<const Json::Value *>(method_call.arguments());
Expand Down