engine icon indicating copy to clipboard operation
engine copied to clipboard

Draft: Move the FlApplication from the template into the linux embedder.

Open jane400 opened this issue 1 year ago • 7 comments

This introduces the class FlApplication, which handles the passing of command line arguments into dart and bringup of a GtkWindow embedding the FlView with for e.g. desktop specifc quirks.

Changes:

This now allows the flutter linux template to be more simple:

#include <flutter_linux/flutter_linux.h>

#include "flutter/generated_plugin_registrant.h"

void view_constructed(FlApplication* app, GtkWindow* window, FlView* view, gpointer user_data) {
  fl_register_plugins(FL_PLUGIN_REGISTRY(view));

  gtk_window_set_title(window, "{{projectName}}");
  gtk_window_set_default_size(window, 1280, 720);
  gtk_widget_show(GTK_WIDGET(window));
}


int main(int argc, char** argv) {
  g_autoptr(FlApplication) app = fl_application_new(APPLICATION_ID, G_APPLICATION_NON_UNIQUE);
  fl_application_connect_view_constructed (app, view_constructed, nullptr);
  return g_application_run(G_APPLICATION(app), argc, argv);
}

I think this is a pretty API, which should be forward compatible with multi view/window flutter applications. Suggestions welcome! :)

I would like to also include titlebar changes in this PR. My plan is to listen for the env variable XDG_CURRENT_DESKTOP equals GNOME instead of assuming GNOME implicitly with use_headerbar = TRUE on all wayland platforms. Should I include this here or make a new PR?

List which issues are fixed by this PR. You must list at least one issue. https://github.com/flutter/flutter/issues/142920

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

I didn't run the testsuite yet, and this probably needs new tests (unless someone tells me otherwise :) ). Will have to look into that. Not all clang-format suggestion are applied, as I disliked some of them. (I'm coming from a GObject dev background and tried to keep some GObject style as outlined in the linux-embedder Exception)

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [ ] I read the Tree Hygiene wiki page, which explains my responsibilities.
  • [x] I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • [ ] I listed at least one issue that this PR fixes in the description above.
  • [ ] I added new tests to check the change I am making or feature I am addined new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing g, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • [x] I updated/added relevant documentation (doc comments with ///).
  • [x] I signed the CLA. (~~Currently getting SEC_ERROR_OCSP_SERVER_ERROR, will try later~~, signed)
  • [ ] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jane400 avatar Feb 22 '24 16:02 jane400

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

flutter-dashboard[bot] avatar Feb 22 '24 16:02 flutter-dashboard[bot]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Feb 22 '24 16:02 google-cla[bot]

/cc @robert-ancell for thoughts.

@jane400 is this still something you're looking into? If so, unfortunately policy requires a signed CLA from contributors as mentioned in the bot reply above. Info in the failed invocation link above.

cbracken avatar Apr 23 '24 23:04 cbracken

The CLA check shouldn't fail with the next reinvocation/update of the MR.

I saw during the creation of this MR, that there were multiple MRs introducing Multi View support into flutter, hence put this MR in my backlog.

I would like to also include titlebar changes in this PR. My plan is to listen for the env variable XDG_CURRENT_DESKTOP equals GNOME instead of assuming GNOME implicitly with use_headerbar = TRUE on all wayland platforms. Should I include this here or make a new PR?

Also waiting for an opinion for this.

jane400 avatar Apr 24 '24 13:04 jane400

I got a chance to actually read the Multi-view runner APIs design documents.

Regarding:

static void activate_cb(FlApplication* app, gpointer user_data) {
  fl_register_plugins(FL_PLUGIN_REGISTRY(fl_application_get_view()));

  GtkWindow* window = fl_application_get_window(app);
  gtk_window_set_title(window, "{{projectName}}");
  gtk_window_set_default_size(window, 1280, 720);
}

We could go for this, but fl_application_get_view and fl_application_get_window might be confusing with FlApplication supporting multi view windows, it should rather be called fl_application_get_initial_view (which may be null).

I would still rather provide a custom signal, where we give the initial view to the user. Especially as g_application_activate can be called multiple times with certain G_APPLICATION_FLAGS.

But one could also make the case that FlApplication should constrain itself to a single window, then this API is perfectly fine, and multi-view users should just use GApplication directly and override ->activate

jane400 avatar May 27 '24 14:05 jane400

I've been thinking about this in regards to multi-window and also delaying the view until the first frame is shown (https://github.com/flutter/flutter/issues/151098). It's clear we want FlApplication to handle all this for most Flutter applications rather than making the template more complicated. I propose this PR is simplified to just do the current behaviour of the template, and then we build from that (as noted in the review comments). @jane400 are you happy to update this PR to that? Thanks.

robert-ancell avatar Aug 15 '24 02:08 robert-ancell