Created attachment 145320 [details] [review]
Create ShellApp, rebase things on it

Created attachment 145350 [details] [review]
Create ShellApp, rebase things on it

Previously, we had ShellAppInfo, which contains fundamental
information about an application, and methods on ShellAppMonitor
to retrieve "live" information like the window list.

AppIcon ended up being used as the "App" class which was painful
for various reasons; among them that we need to handle window
list changes, and some consumers weren't ready for that.

Clean things up a bit by introducing a new ShellApp class in C,
which currently wraps a ShellAppInfo.

AppIcon then is more like the display actor for a ShellApp.  Notably,
the ".windows" property moves out of it.  The altTab code which
won't handle dynamic changes instead is changed to maintain a
cached version.

ShellAppMonitor gains some more methods related to ShellApp now.

In the future, we might consider changing ShellApp to be a GInterface,
which could be implemented by ShellDesktopFileApp, ShellWindowApp.

Then we could axe ShellAppInfo from the "public" API and it would
return to being an internal loss mitigation layer for GMenu.

Note this patch depends on the mutter patch in

https://bugzilla.gnome.org/show_bug.cgi?id=598289

Review of attachment 145350 [details] [review]:

::: js/ui/appDisplay.js
@@ +547,3 @@
+    _init: function(app, isFavorite) {
+
+        BaseWellItem.prototype._init.call(this, app, isFavorite);

extra blank line

::: src/shell-app-monitor.c
@@ +783,3 @@
 
+  /* In the transient case, we already added the window. */
+  _shell_app_add_window (app, window);

I think the comment is more confusing than helpful, since it seems to be saying "this is a bug in the transient case"

@@ +1216,3 @@
+
+  /* We don't hold a ref to these; force floating so we can maintain a
+   * consistent (transfer none)

That is only consistent if you require the caller to always sink the return value, which is weird (at least from C). It would be simpler just make the function (transfer full).

@@ +1227,3 @@
+ * @monitor:
+ *
+ * Returns: (transfer container) (element-type ShellApp): List of favorite applications

likewise this would become (transfer full)

also, everything else favorites-related is on ShellAppSystem... it seems to me like this would make more sense as a ShellAppSystem method that called shell_app_monitor_get_default() rather than as a ShellAppMonitor method that calls shell_app_system_get_default(). (Or maybe we need more serious refactoring.)

::: src/shell-app.c
@@ +118,3 @@
+ * application.  The returned list will be sorted first by whether
+ * they're on the active workspace, and then by the time the user last
+ * interacted with them.

and also by whether they're shown or hidden

@@ +169,3 @@
+
+int
+shell_app_compare (ShellApp *app,

should document what order this returns

@@ +273,3 @@
+
+static void
+shell_app_dispose (GObject *object)

need to disconnect app->workspace_switch_id if non-0?

(In reply to comment #4)
>
> also, everything else favorites-related is on ShellAppSystem... it seems to me
> like this would make more sense as a ShellAppSystem method that called
> shell_app_monitor_get_default() rather than as a ShellAppMonitor method that
> calls shell_app_system_get_default(). (Or maybe we need more serious
> refactoring.)

We do need a more serious refactoring at some point, but the problem is that ShellAppMonitor already depends on ShellAppSystem, and I'd like to avoid making the reverse link too.

Briefly the refactoring would probably look like this:

* Split ShellAppMonitor into
  - ShellWindowTracker (associates MetaWindow -> ShellApp)
  - ShellAppUsage (watches for application focus changes, writes the XML etc)  This one could even move into JS likely (modulo pain of writing XML from JS, maybe we could switch to JSON)
* Entirely replace ShellAppInfo with ShellApp
* ShellWindowTracker calls into ShellAppSystem to notify when windows for an app change, which then updates the ShellApp

Created attachment 145367 [details] [review]
Create ShellApp, rebase things on it

Previously, we had ShellAppInfo, which contains fundamental
information about an application, and methods on ShellAppMonitor
to retrieve "live" information like the window list.

AppIcon ended up being used as the "App" class which was painful
for various reasons; among them that we need to handle window
list changes, and some consumers weren't ready for that.

Clean things up a bit by introducing a new ShellApp class in C,
which currently wraps a ShellAppInfo.

AppIcon then is more like the display actor for a ShellApp.  Notably,
the ".windows" property moves out of it.  The altTab code which
won't handle dynamic changes instead is changed to maintain a
cached version.

ShellAppMonitor gains some more methods related to ShellApp now.

In the future, we might consider changing ShellApp to be a GInterface,
which could be implemented by ShellDesktopFileApp, ShellWindowApp.

Then we could axe ShellAppInfo from the "public" API and it would
return to being an internal loss mitigation layer for GMenu.

Attachment 145367 [details] pushed as 38c06ca - Create ShellApp, rebase things on it

The attached patch is not what was committed. Notably, appDisplay.js:acceptDrop() in git still has a reference to this._arrayValues(), which no longer exists

Created attachment 145454 [details] [review]
[AppWell] Fix D&D for ShellApp

The drag and drop case needed to be updated to use ShellApp
correctly.  Export _is_transient for better compatibility.

Attachment 145454 [details] pushed as d9df7c1 - [AppWell] Fix D&D for ShellApp

Created attachment 145538 [details] [review]
[ShellApp] Fix handler signature for workspace switch

This was causing crashes or undefined behavior.

Comment on attachment 145538 [details] [review]
[ShellApp] Fix handler signature for workspace switch

Attachment 145538 [details] pushed as d705c1b - [ShellApp] Fix handler signature for workspace switch