[linux] Implements basic ASCII text input #46

awdavies · 2018-04-19T20:50:20Z

Text input supports:
-- Deleting highlighted sections of text (highlight must be done w/ the mouse pointer for now).
-- Writing basic ASCII
-- Moving the cursor left and right.
-- Home/End keys to go to the beginning/end of a line.

This change also adds some header guard cleanup since paths have changed.

stuartmorgan-g

Nice!

stuartmorgan-g · 2018-04-24T21:22:22Z

linux/library/include/flutter_desktop_embedding/plugin.h

 #include <string>

 namespace flutter_desktop_embedding {

+typedef std::function<void(const std::string &, const Json::Value &)>


Could you comment this? E.g., meanings of the arguments.

stuartmorgan-g · 2018-04-24T21:28:53Z

linux/library/include/flutter_desktop_embedding/plugin.h

@@ -31,14 +36,24 @@ class Plugin {
  // |input_blocking| Determines whether user input should be blocked during the
  // duration of this plugin's platform callback handler (in most cases this
  // can be set to false).
-  explicit Plugin(std::string channel, bool input_blocking = false)
-      : channel_(channel), input_blocking_(input_blocking) {}
+  explicit Plugin(std::string channel, PlatformCallback platform_callback,


For now (things may change in the API revamp), how about eliminating platform_callback and having the logic for building and sending a response message given the channel and JSON be part of the Plugin base class?

I'm confused about what this means. The plugin can't make a call to the engine unless it has an instance of it. Should we then be including a field with a pointer to the flutter engine in the base class?

Yes, either way it's tied to the engine; I think making it explicit with the engine pointer is reasonable since a plugin is fundamentally a channel for talking to the engine.

Updated this to take a setter method. Unsure if this is what you had in mind.

The client is still responsible providing the callback, not just the engine as discussed above. Summarizing from the offline discussion earlier, my take on this is:

The callback logic is always going to be the same modulo the engine pointer.

That logic is fundamentally about the details of the message format used by the engine.

The core purpose of the Plugin class is to provide a higher-level abstraction around communicating with the engine.
Given that, a protected method within Plugin seems like the right place for that logic to live, rather than requiring something outside of Plugin to implement it and provide that implementation.

Ping on this. I accidentally replied as a separate comment rather than as part of the review, so I'm guessing it was lost.

Alright, got it in. This required a minor change to the file chooser plugin, btw (just need it to call the engine callback instead of returning the info as part of the HandlePlatformMessage response).

stuartmorgan-g · 2018-04-24T21:30:13Z

linux/library/include/flutter_desktop_embedding/plugin_handler.h

@@ -52,10 +57,20 @@ class PluginHandler {
      std::function<void(void)> input_block_cb = [] {},
      std::function<void(void)> input_unblock_cb = [] {});

+  const std::vector<KeyboardHookFunction>& keyboard_hooks() const {


These need declaration comments explaining what they are.

But as with the comments on Plugin, ideally we should move these out of the core class.

Removed! 👍

stuartmorgan-g · 2018-04-24T21:33:19Z

linux/library/src/text_input_plugin.cc

+    int client_id = client_id_json.asInt();
+    if (input_models_.find(client_id) == input_models_.end()) {
+      input_models_.insert(std::make_pair(
+          client_id, std::make_unique<TextInputModel>(client_id)));


Should we cap the size of this? Wasn't there some discussion of the ID always going up over time even for the same set of text fields?

I think we should, yes. However inflating text ID fields should be fixed once we send back valid callback responses via HandlePlatformMessage (instead of returning null).

stuartmorgan-g · 2018-04-24T21:46:29Z

linux/library/include/flutter_desktop_embedding/plugin.h

  virtual ~Plugin() {}

+  // A function for hooking into keyboard input.
+  virtual void KeyboardHook(GLFWwindow *window, int key, int scancode,


Making things specific to the text plugin part of Plugin would be unfortunate, since other plugins shouldn't need this. Since at some point text input will be a specific API rather than part of the plugin system I'd rather we treat this specially up front. We can have this be part of the subclass, and the main embedding code can know about the text input plugin as a specific object rather than just a generic plugin.

Moved to an abstract class that the text input plugin now uses.

stuartmorgan-g · 2018-04-24T21:49:45Z

linux/library/src/flutter_embedder.cc

@@ -72,6 +76,22 @@ static void GLFWwindowSizeCallback(GLFWwindow *window, int width, int height) {
  FlutterEngineSendWindowMetricsEvent(state->engine, &event);
 }

+// Callback invoked when a plugin wants to send a message to the Flutter Engine.
+static void GLFWOnPlatformCallback(GLFWwindow *window,


It seems confusing to have this named using the same style as the engine callbacks. Why not just SendFlutterEngineMessage or something like that?

I was a bit on the fence about the naming of this function myself. I think that works.

stuartmorgan-g · 2018-04-24T21:50:59Z

linux/library/src/flutter_embedder.cc

-      .message_size = output.size(),
-  };
-  FlutterEngineSendPlatformMessage(state->engine, &platform_message_response);
+  // TODO(awdavies): This needs updating for Issue #45.


Do we need this code as part of this patch? The macOS text input plugin always sends back null for now just like the other ones, so I would expect we could leave this out and revisit later.

stuartmorgan-g · 2018-04-24T21:52:00Z

linux/library/src/text_input_model.cc

+
+#include <flutter_desktop_embedding/common/platform_protocol.h>
+
+static constexpr char kComposingBaseKey[] = "composingBase";


Let's put a comment referencing #47 on these constants (which I'll do on the macOS side as well sometime soon). I want to make sure anyone looking at the code is aware that this isn't actually supported.

stuartmorgan-g · 2018-04-24T21:53:00Z

linux/library/include/flutter_desktop_embedding/text_input_model.h

+ private:
+  void DeleteSelected();
+
+  std::string rep_;


No abbreviations in variable names, per style guide.

stuartmorgan-g · 2018-04-24T22:00:32Z

linux/library/src/text_input_model.cc

+
+void TextInputModel::DeleteSelected() {
+  selection_extent_ = rep_.erase(selection_base_, selection_extent_);
+  selection_base_ = selection_extent_;


Per offline discussion let's change to:
selection_base_ = ...
selection_extent_ = selection_base_
since it seemed more confusing to me as written.

Same with some other spots below.

Fixed! 😀

awdavies · 2018-04-30T20:54:31Z

@stuartmorgan PTAL

Addressed comments.

stuartmorgan-g

Just a couple notes left; looking really good!

stuartmorgan-g · 2018-05-01T21:36:36Z

linux/library/include/flutter_desktop_embedding/text_input_plugin.h

+
+  void KeyboardHook(GLFWwindow *window, int key, int scancode, int action,
+                    int mods) override;
+


Given that there are multiple parents, I'd use labelled clusters for the overrides to make it clearer what's what:

// Plugin
Json::Value HandlePlatformMessage(const Json::Value &message) override;

// KeyboardHookHandler:
void KeyboardHook(GLFWwindow *window, int key, int scancode, int action,
int mods) override;
void CharHook(GLFWwindow *window, unsigned int code_point) override;

Sounds good! Of all the C++ I've done this is the first time I've actually used multiple inheritance

stuartmorgan-g · 2018-05-01T21:40:56Z

linux/library/src/flutter_embedder.cc

+        .message = reinterpret_cast<const uint8_t *>(output.c_str()),
+        .message_size = output.size(),
+    };
+    FlutterEngineSendPlatformMessage(state->engine, &platform_message_response);


Since this is all duplicate code with line 89, extract to a static helper? (Eventually I think this will move into Plugin, but I can revisit that when reworking the plugin API across platforms.)

I think just calling the static function would be sufficient, no? The code is the same, for sure.

awdavies · 2018-05-02T18:38:12Z

@stuartmorgan Last pass! (maybe)

stuartmorgan-g · 2018-05-03T22:16:16Z

linux/library/include/flutter_desktop_embedding/plugin.h

+// Contains a channel string for the plugin and a JSON value to pass to the
+// engine.
+typedef std::function<void(const std::string &, const Json::Value &)>
+    PlatformCallback;


This can be removed now, right?

Woops! Forgot that was there. Good catch

stuartmorgan-g · 2018-05-03T22:17:17Z

linux/library/include/flutter_desktop_embedding/plugin.h

+  virtual void set_flutter_engine(FlutterEngine engine) { engine_ = engine; }
+
+ protected:
+  // Runs the platform callback. Noop if the callback is not set.


This comment is out of date. Also, maybe this function should be called SendMessageToEngine or something similar now, since it's not really clear what CallPlatformCallback means in this structure.

stuartmorgan-g · 2018-05-03T22:20:47Z

linux/library/include/flutter_desktop_embedding/plugin.h

+
+ protected:
+  // Runs the platform callback. Noop if the callback is not set.
+  void CallPlatformCallback(const Json::Value &json) {


Let's move this to a .cc, since it doesn't need to be in the header and we don't want it inlined.

CC file made!

awdavies added 3 commits April 18, 2018 18:21

Initial implementation.

a717554

Completed implementation.

cc49532

Added missing header.

620791a

awdavies changed the title ~~Implements basic ASCII text input~~ [linux] Implements basic ASCII text input Apr 19, 2018

awdavies requested a review from stuartmorgan-g April 19, 2018 20:53

Reverted example directory.

fccafea

stuartmorgan-g reviewed Apr 24, 2018

View reviewed changes

awdavies added 8 commits April 27, 2018 12:16

Text input model fixes.

9cb84f7

Update constants.

5eb5e64

Marked constants w/ constexpr

9454d33

Update plugin API.

32fe54e

Remove keyboard hooks from plugin API.

98578f1

Update callback function name.

bd8f252

Comment removed.

fe085b1

Add text input model limit.

229217f

stuartmorgan-g reviewed May 1, 2018

View reviewed changes

More comments addressed.

752dc1f

Change from callback to flutter engine pointer.

a078aab

stuartmorgan-g requested changes May 3, 2018

View reviewed changes

Move plugin funcs out of inline defs.

8681ea8

awdavies force-pushed the text_input_mk2 branch from c006795 to 8760395 Compare May 3, 2018 22:38

Function names fixed. unused def removed.

f9fb0a7

awdavies force-pushed the text_input_mk2 branch from 8760395 to f9fb0a7 Compare May 3, 2018 22:38

camel-case fix

9483d44

stuartmorgan-g approved these changes May 3, 2018

View reviewed changes

awdavies merged commit 688213e into google:master May 3, 2018

stuartmorgan-g mentioned this pull request Jun 7, 2018

Add text input support to Linux #24

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2021


		#include <flutter_desktop_embedding/common/platform_protocol.h>

		static constexpr char kComposingBaseKey[] = "composingBase";


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

[linux] Implements basic ASCII text input #46

[linux] Implements basic ASCII text input #46

Conversation

awdavies commented Apr 19, 2018

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awdavies Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awdavies commented Apr 30, 2018

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awdavies commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awdavies Apr 24, 2018 •

edited

Loading