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

@robert-ancell robert-ancell commented Mar 6, 2020

This is a start on a Linux shell, and only supports rendering of Flutter, no
input is currently provided. The API is expected to change as we flesh out
the shell. The shell doesn't build by default yet, to enable it you need to
set the build_linux_flag gn build argument.

This is the first stage of flutter/flutter#30729

The shell only works on X servers, the EGL code will need to be updated to cover
Wayland.

The Flutter engine currently crashes when the widget is destroyed, but this
seems to be an issue in the engine.

The code is designed to match GTK conventions.

A minimal GTK application that uses this looks like:

int
main (int argc, char **argv)
{
    gtk_init (&argc, &argv);

    GtkWidget *window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
    gtk_widget_show (window);

    g_autoptr(FlDartProject) project = fl_dart_project_new ("./build/flutter_assets", "./linux/flutter/ephemeral/icudtl.dat");
    g_autoptr(FlView) view = fl_view_new (project);

    gtk_widget_show (GTK_WIDGET (view));
    gtk_container_add (GTK_CONTAINER (window), GTK_WIDGET (view));

    gtk_main ();

    return 0;
}

@auto-assign auto-assign bot requested a review from cbracken March 6, 2020 10:28
@stuartmorgan-g stuartmorgan-g self-requested a review March 6, 2020 10:32
@robert-ancell robert-ancell force-pushed the linux-shell branch 6 times, most recently from 32dc86e to 8182418 Compare March 7, 2020 21:44
@chinmaygarde chinmaygarde self-requested a review March 10, 2020 05:04
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.

Sorry for the long delay; I was OOO for several days.

Capturing from some offline discussion for anyone looking back at this wondering about the style: because this is intended to be consumed at the GTK level, we have a fairly strong constraint to follow GTK style to avoid it being extremely alien to anyone familiar with GTK conventions. Unfortunately that means we have to violate the Google C++ style guide in a number of ways (function naming, use of macros in API declaration). Given that we have to do that in the API, we're being consistent about those things in implementation details as well to avoid the internals being too much of a hybrid that is hard to understand for someone used to either style.

However, where there are internal-only details that aren't strongly at odds with the rest of the style, we'll follow Google C++ style. Examples:

  • Using C++, not pure C, for the implementation files.
  • Forbidding C-style casts.
    But we won't, for instance, use Google-style (camel case) function names for static functions in the same files as we're using snake case for the public functions.

@robert-ancell robert-ancell force-pushed the linux-shell branch 3 times, most recently from ae0c022 to 4872951 Compare March 13, 2020 03:58
This is a start on a Linux shell, and only supports rendering of Flutter, no
input is currently provided. The API is expected to change as we flesh out
the shell. The shell doesn't build by default yet, to enable it you need to
set the build_linux_flag gn build argument.

This is the first stage of flutter/flutter#30729

The shell only works on X servers, the EGL code will need to be updated to cover
Wayland.

The Flutter engine currently crashes when the widget is destroyed, but this
seems to be an issue in the engine.

The code is designed to match GTK conventions.

A minimal GTK application that uses this looks like:

--------

int
main (int argc, char **argv)
{
    gtk_init (&argc, &argv);

    GtkWidget *window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
    gtk_widget_show (window);

    g_autoptr(FlDartProject) project = fl_dart_project_new ("./build/flutter_assets", "./linux/flutter/ephemeral/icudtl.dat");
    g_autoptr(FlView) view = fl_view_new (project);

    gtk_widget_show (GTK_WIDGET (view));
    gtk_container_add (GTK_CONTAINER (window), GTK_WIDGET (view));

    gtk_main ();

    return 0;
}
--------

G_DECLARE_FINAL_TYPE(FlDartProject, fl_dart_project, FL, DART_PROJECT, GObject)

FlDartProject* fl_dart_project_new(const gchar* assets_path,
Copy link
Contributor

Choose a reason for hiding this comment

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

As a follow-up PR, it would be good to move this to align with #17210. At the level of this API, we should enforce a folder structure for all the artifacts we expect (both to simplify the API, and to make it more future-proof), and since we're writing the tooling that people will be expected to use to generate that folder they won't even need to know the details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made this change in another branch, but can fold it into this PR if you like.

@stuartmorgan-g stuartmorgan-g added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 24, 2020
@stuartmorgan-g
Copy link
Contributor

Filed flutter/flutter#53176 for the work necessary to have this build by default. The earlier that happens the better, as it'll give us bot coverage for the GTK PRs.

@fluttergithubbot fluttergithubbot merged commit c93b67a into flutter:master Mar 24, 2020
@robert-ancell
Copy link
Contributor Author

I noticed the commit message is completely different - is that expected?

@stuartmorgan-g
Copy link
Contributor

I noticed the commit message is completely different - is that expected?

Ugh, I forgot that the 'waiting' label triggered the auto-commiter. It has terrible commit message handling at the moment (which is the main reason I don't usually use it).

Luckily our commits point back to the PRs, so it's not too hard to get better info. But I do generally put the PR description into the commit message instead of the auto-generated squash message.

@robert-ancell
Copy link
Contributor Author

This is being reverted, right? Should I make a new PR after that occurs?

@stuartmorgan-g
Copy link
Contributor

No, there was some confusion because an infra failure on the bots happened to show up on the build with this PR. It shouldn't actually be reverted.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants