Created attachment 132599 [details] [review]
Abstract Alt-Tab handling out into a helper class

The default implementation just uses the old MetaTabPopup, and the
workspace switcher still always uses that, for now.

Created attachment 132600 [details] [review]
First attempt at Alt-Tab handling

Is there a reason to have the default implementation and keep the old src/ui/tabpopup.c around?  Can we just say that mutter consumers are responsible for implementing alt-tab visualizations?

well, for the moment, the workspace-switching keys still use src/ui/tabpopup.c

I guess it depends on what o-hand are doing. If they don't need the default implementation (and I think they don't) then we can probably drop it.

Other quick comments:

* Constrain the proportions of the popup and ellipsize?  It gets sort of misshapen if one has a window with a long title.

* +  g_ptr_array_free (sth->windows, FALSE);

Should be TRUE, no?  I don't think anything else is using the segment.

* A comment about the relationship between these classes somewhere would be nice.  Maybe at the top of altTab.js.

(In reply to comment #5)
> * Constrain the proportions of the popup and ellipsize?  It gets sort of
> misshapen if one has a window with a long title.

Yeah, I just didn't want to spend too much time fiddling with the effects since we probably want something a little sexier eventually anyway. Also, tweaking the specific clutter effects is easier for casual contributors than figuring out the mutter-integration bits, so I figure other people might tackle that anyway.

> * +  g_ptr_array_free (sth->windows, FALSE);
> 
> Should be TRUE, no?  I don't think anything else is using the segment.

oops, yeah

Created attachment 132840 [details] [review]
First attempt at Alt-Tab handling

This fixes the layout a bit, and ellipsizes too-long labels. The code
is getting pretty ugly already... BigBox doesn't *quite* do what we want...

If we consider this too be three pieces:

 A) Metacity additions
 B) gnome-shell C adapter object to make up for gjs deficiencies
 C) gnome-shell javascript

then it feels to me like some of the things that I'd expect to be in B):

 - Having a concrete class
 - Having properties

Are actually in A) instead. Admitting that all GObject code in C is a pain to write, it doesn't immediately seem harder to have MetaAltTab handler be an interface, and MetaAlTabHandlerDefault and ShellAltTabHandler objects that implement it.

(I see that you are also using as factory arguments, see below)

Some other comments about the Metacity code:

+MetaAltTabHandler *
+meta_alt_tab_handler_new (MetaScreen *screen,
+                          gboolean    immediate)
+{
+  if (handler_type == G_TYPE_INVALID)
+    handler_type = meta_alt_tab_handler_default_get_type ();
+
+  return g_object_new (handler_type,
+                       "screen", screen,
+                       "immediate", immediate,
+                       NULL);
+}

This - using a GType as the "factory" - is a pattern that I tend to avoid, because it binds poorly to many language bindings (though gjs may be OK) and doesn't accomodate "user data" when creating the object.

Is there any reason we can't just have:

 meta_screen_set_alt_tab_handler(screen, handler)

? (with a default being created on first use if none have been set by then.)

The counter-argument here is:

 - It probably *will* be OK in gjs - that is, Javascript has first class types as much as it has types as all, so AltTabHander.g_type is possible.
 - Sometimes we might actually want a factory; imagine something really heavyweight that we don't want to create on startup.
 - Other approaches to factories are incredibly annoying from GObject/C - you really don't want to have to create *two* types.
 - User data could be set on the screen

If we want to a) eventually use interfaces b) use GTypes as factories c) pass arguments by properties, then we'll have to either duck-type the properties or use properties on interfaces... with the the latter likely being the better choice (We'd need support in GJS for g_object_class_override_property()) The simpler alternative is to just have an init() method and avoid properties.

Just a few other comments on the Metacity changes:

 - Maybe stick to one GType per file, rather than combining the abstract and default implementations together?

- wasn't clear to me where the preview/get_window_pixbuf() stuff went. Maybe just needs to be commented in the log message.

(In reply to comment #8)
> If we consider this too be three pieces:
> 
>  A) Metacity additions
>  B) gnome-shell C adapter object to make up for gjs deficiencies
>  C) gnome-shell javascript
> 
> then it feels to me like some of the things that I'd expect to be in B):
> 
>  - Having a concrete class
>  - Having properties
> 
> Are actually in A) instead. Admitting that all GObject code in C is a pain to
> write, it doesn't immediately seem harder to have MetaAltTab handler be an
> interface, and MetaAlTabHandlerDefault and ShellAltTabHandler objects that
> implement it.

Yeah... I've forgotten now why I didn't do it that way. I definitely considered it at one point. And given what I said about wanting altTab.js to implement an interface directly, that makes even more sense, because, assuming gjs gets better GObject support in the future (or else we switch to seed), then we can just remove (B) and not need to change (A) at all.

> Is there any reason we can't just have:
> 
>  meta_screen_set_alt_tab_handler(screen, handler)

You mean where "handler" is actually a MetaAltTabHandler object rather than some sort of constructor for MetaAltTabHandler objects? The old code created a new tab-handling-thingy each time the user hit Alt-Tab, so I did the factory thing so it could keep working that way. But I guess we could say that meta_alt_tab_handler_hide() means "throw away the list of windows" too, so that it could then keep reusing the same one.

> - wasn't clear to me where the preview/get_window_pixbuf() stuff went. Maybe
> just needs to be commented in the log message.

It's gone. AFAICT it was part of the original metacity compositor code that wasn't being used by mutter.

I love the patch.  I would say that the behaviour on a dualhead configuration is a bit unfortunate (box spans the two screens, smack in the middle of the virtual desktop).  Still, intensely good looking :)

(In reply to comment #9)
> > Is there any reason we can't just have:
> > 
> >  meta_screen_set_alt_tab_handler(screen, handler)
> 
> You mean where "handler" is actually a MetaAltTabHandler object rather than
> some sort of constructor for MetaAltTabHandler objects? The old code created a
> new tab-handling-thingy each time the user hit Alt-Tab, so I did the factory
> thing so it could keep working that way. But I guess we could say that
> meta_alt_tab_handler_hide() means "throw away the list of windows" too, so that
> it could then keep reusing the same one.

As mentioned on Friday, this ended up being basically impossible without cleaning up the underlying code in metacity. So I did that, but then once I started implementing this, I realized it really doesn't make any sense at all; you end up having to have a meta_alt_tab_handler_is_visible() method, and replacing various screen->tab_handler!=NULL checks with meta_alt_tab_handler_is_visible(screen->tab_handler) instead, not to mention the somewhat odd "hide = restart" semantics mentioned above, etc. So after doing all the cleanup, I think ended up implementing this exactly like I did before, with a GType factory. Of course, we can change the exact nature of the factory. (It could be a GType+gpointer user_data, or you could set a constructor function instead of a gtype, or whatever.)

Of course, the cleanups are nice anyway. Patches to be attached. Once we decide we're happy with the general idea, I'll refile the mutter parts as a mutter bug.

Created attachment 133499 [details] [review]
Remove some old compositor-related code that mutter doesn't need

Created attachment 133500 [details] [review]
Fix some broken indentation and simplify nested if()s

Created attachment 133501 [details] [review]
Reorganize tab popup code a bit more cleanly.

Created attachment 133502 [details] [review]
Split screen->tab_popup and screen->ws_popup

Created attachment 133503 [details] [review]
Push the tab/workspace popup abstraction completely into screen.c

Created attachment 133504 [details] [review]
Create a MetaAltTabHandler abstraction to allow alternate implementations

Created attachment 133505 [details] [review]
Third attempt at Alt-Tab handling

(In reply to comment #12)
> Created an attachment (id=133499) [edit]
> Remove some old compositor-related code that mutter doesn't need

Looks good to me, maybe should be combined with removing the get_window_pixmap() hook all-together, but that really is best done after removing compositor-xrender altogether.

(In reply to comment #13)
> Created an attachment (id=133500) [edit]
> Fix some broken indentation and simplify nested if()s

Looks good, trusting you a bit that the logic is unchanged (since it's a little hard to figure out the old indentation from the patch)

(In reply to comment #14)
> Created an attachment (id=133501) [edit]
> Reorganize tab popup code a bit more cleanly.

Looks likes a nice cleanup, maybe could do with a bit more of a verbose commit message about what's going on in the patch.

(In reply to comment #15)
> Created an attachment (id=133502) [edit]
> Split screen->tab_popup and screen->ws_popup

Looks good.

(In reply to comment #16)
> Created an attachment (id=133503) [edit]
> Push the tab/workspace popup abstraction completely into screen.c

Looks like a nice improvement.

meta_screen_ensure_workspace_popup (MetaScreen    *screen,
+                                    MetaWorkspace *initial_selection)

Hmm, having an "ensure" function with an argument is a bit weird... the expectation for an ensure function is that if the thing that you are ensuring already exists, it does nothing. Which is what happens here. But then what effect does the argument have? (Your patch seems to have a change from "always select" to "do nothing") May just need a comment explaining the handling of an already existing popup.

The change from using the XWindow to using the MetaWindow as the MetaTabEntryKey should be described in the commit message, or otherwise parts of this patch are hard to understand.

(In reply to comment #17)
> Created an attachment (id=133504) [edit]
> Create a MetaAltTabHandler abstraction to allow alternate implementations

Generally looks very good. The patch makes a lot more sense to read on top of the cleanups.

Maybe there should be function docs on the MetaAltTabHandler? - I think ideally everything that is exported in include/ would have docs.

And one nit-pick:

+  MetaTabPopup *tabpopup;

SHould be tab_popup I think, consistently with what was elsewhere and the TabPopup naming.

I think all of these patches (with whatever improvements you want to make) could be filed on a single mutter bug (or maybe two - one for the cleanup, one for MetaAltTabHandler). You don't need to wait for additional review there (just provide a pointer to this bug), but it would be good to wait 24 hours or so after filing before committing in case anybody wants to provide feedback.

The gnome-shell patch looks good to me. I'd like to see a comment somewhere explaining the ShellAltTabHandler / ShellWM relation and maybe giving some insight how this will work in a better future.

+        // BigBox complains if you put an ellipsized ClutterText into it
+        // directly! :-/

This is something I don't much like seeing without a bug URL. We shouldn't just live with the weirdnesses and bugs of BigBox, we should work to make sure that they get fixed, so we don't have to code around them.

mutter bits committed as bug 580917

and gnome-shell bits committed

(In reply to comment #25)
> The gnome-shell patch looks good to me. I'd like to see a comment somewhere
> explaining the ShellAltTabHandler / ShellWM relation and maybe giving some
> insight how this will work in a better future.

done. (in shell-alttab.c)

> +        // BigBox complains if you put an ellipsized ClutterText into it
> +        // directly! :-/
> 
> This is something I don't much like seeing without a bug URL. We shouldn't just
> live with the weirdnesses and bugs of BigBox, we should work to make sure that
> they get fixed, so we don't have to code around them.

The bug appears to have gone away since I wrote that comment. Probably fixed by the clutter text-actor-layout-height merge.