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

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

Closed
wants to merge 1 commit into from

Conversation

jane400
Copy link

@jane400 jane400 commented Feb 22, 2024

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.
flutter/flutter#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)

  • 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.
  • 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.
  • I updated/added relevant documentation (doc comments with ///).
  • 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.

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.
@flutter-dashboard
Copy link

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.

Copy link

google-cla bot commented Feb 22, 2024

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.

@cbracken
Copy link
Member

cbracken commented Apr 23, 2024

/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.

@jane400
Copy link
Author

jane400 commented Apr 24, 2024

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.

@robert-ancell robert-ancell self-requested a review April 29, 2024 01:35
Copy link
Contributor

@robert-ancell robert-ancell left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I think this can be a bit simpler and more GTK style, and the usage would be more like this:

#include <flutter_linux/flutter_linux.h>

#include "flutter/generated_plugin_registrant.h"

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);
}

int main(int argc, char** argv) {
  g_autoptr(FlApplication) app = fl_application_new(APPLICATION_ID, G_APPLICATION_NON_UNIQUE);
  g_signal_connect(app, "activate", G_CALLBACK(activate_cb), nullptr);
  return g_application_run(G_APPLICATION(app), argc, argv);
}

// like signaling the application that it should show itself again.
// This only stops the same instance of GApplication creating multiple
// windows/views.
if (priv->activated) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why this is required - activate() is only ever called once isn't it?

Copy link
Author

@jane400 jane400 Apr 29, 2024

Choose a reason for hiding this comment

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

If the application is guaranteed to be unique (by not giving GApplication the flag non-unique) a second instance will send the argv to the main instance over the dbus and run activate there again. The gtk4 template in gnome-builder can be used to reproduce this.

Other things can also activate it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at all existing GTK apps I can find they don't seem to do this - if you call activate a second instance of the main window is shown, which is what Flutter applications do as well. I think we should match the existing behaviour and propose this as a second change if it's required.


gtk_widget_grab_focus(GTK_WIDGET(view));

g_signal_emit (G_OBJECT (self),
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be necessary - a Flutter application can just override the activate method (chaining up to the FlApplication implementation) or connect to the GApplication::activate signal.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been thinking how this would work for multi-window. I think the user needs the ability to create the window themselves in this signal, as a multi-window app might have special windows that use GTK widgets. Since this is complicated and needs to be well thought out let's drop this from this PR and resolve in follow up PRs.

@jane400
Copy link
Author

jane400 commented May 27, 2024

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

@robert-ancell
Copy link
Contributor

I've been thinking about this in regards to multi-window and also delaying the view until the first frame is shown (flutter/flutter#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.

@Piinks
Copy link
Contributor

Piinks commented Oct 15, 2024

(PR Triage): What is the status of this PR?

@goderbauer
Copy link
Member

(triage): I am going to close this one since it looks stale. If you find the time to work on this again feel free to open a new PR. Thank you.

@goderbauer goderbauer closed this Dec 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants