Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

robert-ancell
Copy link
Contributor

No description provided.

@robert-ancell
Copy link
Contributor Author

This is a first iteration on the platform message support and allows sending of messages in both directions. It is built on top of #17910. This is the MVP of this feature, submitting here for feedback.

An example on these new APIs is:

static FlView *view;

static void
message_cb (FlEngine *engine, const gchar *channel, GBytes *message, FlEnginePlatformMessageHandle *handle, gpointer user_data)
{
    g_autoptr(GBytes) response = do_something_with_message (message);

    g_autoptr(GError) error = NULL;
    if (!fl_engine_send_platform_message_response (engine, handle, response, &error))
        g_printerr ("Failed to send platform message response: %s\n", error->message);
}

static void
message_response_cb (GObject *object, GAsyncResult *result, gpointer user_data)
{
    g_autoptr(GError) error = NULL;
    g_autoptr(GBytes) response = fl_engine_send_platform_message_finish (fl_view_get_engine (view),
                                                                         result,
                                                                         &error);
   
    if (response == NULL) {
        g_printerr ("Failed to send platform message: %s\n", error->message);
        return;
    }

    do_something_with_response (response);
}

static void
some_external_event ()
{
    g_autoptr(GBytes) message = make_request ();

    fl_engine_send_platform_message (fl_view_get_engine (view),
                                     channel,
                                     message,
                                     NULL,
                                     message_response_cb,
                                     NULL);
}

int
main (int argc, char **argv)
{
    view = set_up_gtk ();

    g_autoptr(FlDartProject) project = fl_dart_project_new (".");
    view = fl_view_new (project);

    fl_engine_set_platform_message_handler (fl_view_get_engine (view),
                                            "example.com/gtk-test-method",
                                            message_cb,
                                            NULL);

    gtk_main ();

    return 0;
}

I've intentionally kept the APIs simple and as close to the GLib/GTK patterns. Do we then build the higher level codec/plugins on top of this or restructure it? Should we land this as working but refactor it later if the higher level code requires the API to change?

cc @stuartmorgan

@stuartmorgan-g stuartmorgan-g self-requested a review April 28, 2020 15:52
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

An example on these new APIs is

Thanks, that was super-helpful in understanding the code :)

I've intentionally kept the APIs simple and as close to the GLib/GTK patterns. Do we then build the higher level codec/plugins on top of this or restructure it? Should we land this as working but refactor it later if the higher level code requires the API to change?

Other than my comment inline about moving the APIs off of the engine's API surface, the APIs and the example look good to me. (I have some questions inline, but they are "how do I GLib/GTK?" questions, not things I think need to be changed.) The primitives here look basically the same as the primitives used in the Windows and GLFW embeddings as the foundation for the higher-level channel APIs, so I wouldn't expect it to need refactoring later. Even languages where we don't have the the requirement to expose them (as with the C/C++ split in Windows and GLFW), these primitives are still exposed as public API in addition to the higher-level APIs (see for example FlutterBinaryMessenger on iOS/macOS).

@stuartmorgan-g
Copy link
Contributor

cc @stuartmorgan

FYI now that you have project permissions you can just add me as a reviewer directly.

@robert-ancell robert-ancell force-pushed the linux-shell-platform-message branch 3 times, most recently from 9907e35 to 2167bbd Compare April 29, 2020 03:24
@robert-ancell
Copy link
Contributor Author

@stuartmorgan I've added FlBinaryMessenger and tried to match the MacOS behaviour as much as possible. The only remaining issue is if we need to copy the message buffer.

The example code using this new API is:

static FlView *view;

static void
message_cb (FlBinaryMessenger *messenger, const gchar *channel, GBytes *message, FlBinaryMessengerResponseHandle *handle, gpointer user_data)
{
    g_autoptr(GBytes) response = do_something_with_message (message);

    g_autoptr(GError) error = NULL;
    if (!fl_binary_messenger_send_response (messenger, handle, response, &error))
        g_printerr ("Failed to send platform message response: %s\n", error->message);
}

static void
message_response_cb (GObject *object, GAsyncResult *result, gpointer user_data)
{
    g_autoptr(GError) error = NULL;
    g_autoptr(GBytes) response = fl_binary_messenger_send_on_channel_finish (FL_BINARY_MESSENGER(object),
                                                                             result,
                                                                             &error);
   
    if (response == NULL) {
        g_printerr ("Failed to send platform message: %s\n", error->message);
        return;
    }

    do_something_with_response (response);
}

static void
some_external_event ()
{
    g_autoptr(GBytes) message = make_request ();

    fl_binary_messenger_send_on_channel (fl_engine_get_binary_messenger(fl_view_get_engine (view)),
                                         channel,
                                         message,
                                         NULL,
                                         message_response_cb,
                                         NULL);
}

int
main (int argc, char **argv)
{
    view = set_up_gtk ();

    g_autoptr(FlDartProject) project = fl_dart_project_new (".");
    view = fl_view_new (project);

    fl_binary_messenger_set_message_handler_on_channel (fl_engine_get_binary_messenger(fl_view_get_engine (view)),
                                                        "example.com/gtk-test-method",
                                                        message_cb,
                                                        NULL);

    gtk_main ();

    return 0;
}

@robert-ancell robert-ancell force-pushed the linux-shell-platform-message branch 3 times, most recently from 727489c to 09469d4 Compare April 29, 2020 21:12
@robert-ancell
Copy link
Contributor Author

Updated to copy the message based on discussion on Discord.

@robert-ancell robert-ancell force-pushed the linux-shell-platform-message branch 5 times, most recently from 73bbcb2 to aca72a0 Compare April 30, 2020 23:19
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM! (I'm having a harder time following the GLib constructions than usual today, so take that with a grain of salt. We can iterate if anything comes up during the higher-level messaging APIs though.)

That reminds me that we should talk about unit testing soon. The desktop embedding are woefully undertested right now, but at least the C++ wrapper has unit tests; as the code here approaches that abstraction level we should figure out tests sooner rather than later so we're not regressing even relative to the minimal testing we currently have with the GLFW embedding.

@robert-ancell robert-ancell force-pushed the linux-shell-platform-message branch from aca72a0 to 833cc7b Compare May 3, 2020 23:01
@robert-ancell
Copy link
Contributor Author

I've also been uneasy about the lack of tests - I've opened flutter/flutter#56261 to track this.

@robert-ancell robert-ancell merged commit 180a497 into flutter:master May 4, 2020
@robert-ancell robert-ancell deleted the linux-shell-platform-message branch May 4, 2020 05:03
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 4, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants