Created attachment 234408 [details] [review]
Add Gtk.Application subclass for minor cleanup

Ideally we'd like Boxes.App do inherit from Gtk.Applicaton, but we can't
as it needs to inherit Boxes.UI. Instead we create a subclass as
a proxy object that calls into Boxes.App. This makes the "app"
callbacks real methods rather than signal connections which look
slightly nicer, and allows later overriding of Gtk.Application vfuncs.

Created attachment 234409 [details] [review]
App: Add call_when_ready

This makes it easy to queue calls until the app is in the ready
state.

Created attachment 234410 [details] [review]
Proper support for command line args in secondary process

We new handle most of the command line parsing (except a few
specialized things) in the primary instance, by implementing the
command_line signal of GApplication. This means we now support
arguments like --open-uuid in the non-primary instance, as needed
for e.g. gnome-shell search results.

This additionally moves the UI initialization and window to
the activate state so that it only happens in the primary instance,
and it doesn't happen if we exit early in the command_line handler.

Unfortunately we can't use the automatic --help support in GOptions
anymore as that prints to/exists the primary instance rather than
the calling one, so we have to now manually do that.

Review of attachment 234408 [details] [review]:

Looks good.

::: src/app.vala
@@ +11,3 @@
+// Ideally Boxes.App should inherit from, Gtk.Application, but we can't also inherit from Boxes.UI,
+// so we make it a separate object that calls into Boxes.App
+private class Boxes.Application: Gtk.Application {

Normally its OK to keep small private classes in its (main) user module but in case of App, I'd want to avail every opportunity to take code out of this module as its bloated IMO.

Review of attachment 234409 [details] [review]:

ack

Review of attachment 234410 [details] [review]:

typo in commit log: new -> now. Rest looks good.

::: src/app.vala
@@ +145,3 @@
+        string [] args = {};
+        unowned string [] args2 = args;
+        GtkClutter.init (ref args2);

i don't get these 3 lines. We want to disable GtkClutter.init's commandline args?

@@ +192,3 @@
     }
 
+    static bool opt_fullscreen;

If you agree that we should put Boxes.Application in a separate file, I'd recommend we do the commandline parsing in that class as well.

Review of attachment 234408 [details] [review]:

::: src/app.vala
@@ +11,3 @@
+// Ideally Boxes.App should inherit from, Gtk.Application, but we can't also inherit from Boxes.UI,
+// so we make it a separate object that calls into Boxes.App
+private class Boxes.Application: Gtk.Application {

I don't agree. I think Boxes.App should really be inherit from Gtk.Application and its primary purpose should be to do things like argument parsing and handle app global things. Unfortunately we can't do this and also inherit from Boxes.UI, as GObject doesn't do multiple inheritance. So, I'm doing this with a proxy object that just calls into the real object.

If anything its the UI code that should be moved out of Boxes.Application. Like, we should subclass Gtk.ApplicationWindow and move a bunch of UI specific code there.

Review of attachment 234410 [details] [review]:

::: src/app.vala
@@ +145,3 @@
+        string [] args = {};
+        unowned string [] args2 = args;
+        GtkClutter.init (ref args2);

Since we don't parse early the Gtk initialization is handled inside GtkApplication.startup(), and does not support the Gtk+ arguments. This is i think were we're generally moving with GApplication based single-instance apps anyway, as most of those arguments don't make sense in that model.

However, Gtk.Application just inits Gtk, so we need to manually initit clutter-gtk too.

@@ +192,3 @@
     }
 
+    static bool opt_fullscreen;

I disagree with that :)

Review of attachment 234408 [details] [review]:

::: src/app.vala
@@ +11,3 @@
+// Ideally Boxes.App should inherit from, Gtk.Application, but we can't also inherit from Boxes.UI,
+// so we make it a separate object that calls into Boxes.App
+private class Boxes.Application: Gtk.Application {

Yup, in this specific case, it makes sense to me to keep the class there. app.vala is indeed one of our bigger files, but at less than 1000 lines of code, it's not terribly huge either.

Created attachment 234600 [details] [review]
Split out window code from App to AppWindow

Move all UI code out of App into AppWindow, making App derive
from Gtk.Application rather than Boxes.UI. This makes the app.vala
file smaller, and makes things easier to find. It also separates the UI
state from the app global stuff, making way to a possible multi-window
boxes.

Unfortunately a lot of stuff referes to the UI state via App.app (and now
App.app.window) which is rather iffy. It would probably be nicer to push
a reference to the window into all the UI subclasses and use that to get
the UI state. Makes for shorter better isolated code.

This is a not very tested patch and is mostly to show off the possibilities
here.

Review of attachment 234600 [details] [review]:

Yeah, I like the idea of this patch. Ver fond of splits I guess. :) I'll review in detail once you have tested it and renamed 'appwindow.vala' to 'app-window.vala'. :)

Attachment 234408 [details] pushed as 1ce5fa8 - Add Gtk.Application subclass for minor cleanup
Attachment 234409 [details] pushed as 7ce3c4e - App: Add call_when_ready
Attachment 234410 [details] pushed as 0d6c295 - Proper support for command line args in secondary process

Closing this bug, moving the split app.vala to bug 692708