Created attachment 154911 [details] [review]
[panel] Fix AppPanelMenu's allocation

Because we were setting the "fixed-position-set" property
on the internal label, its width/height requests weren't
being accounted for in the size request of the overall box.

The way we were hooking up to notify::allocation was hacky; do
this correctly by simply implementing a container.

Created attachment 154912 [details] [review]
[panel] Handle async load of icons correctly

Fading the application icon required the texture to have already been
loaded, which was normally the case since we create icons for apps
in the well and browser; but when finding an app not from there,
the cogl-texture for the icon might not be available.

Fix this by watching for the texture and fading when it appears.

Created attachment 154913 [details] [review]
[windowTracker] Fix tracking of single-process multi app cases

(In reply to comment #0)
> When I open the openoffice document eg. impress presentation the title on the
> top bar states "OpenOffice.org Impress". But when I *in the openoffice* choose
> to create an eg. writer (odt) document the title on the top bar is still the
> same eg. "OpenOffice.org Impress" not "OpenOffice.org Writer". 
> 
> The window decoration title is correct eg. "presentation.odp - OpenOffice.org
> Impress" and in the other document "document.odp - OpenOffice.org Writer".

To add some context ... the three patches together should fix the issue.

Comment on attachment 154911 [details] [review]
[panel] Fix AppPanelMenu's allocation

>+        this.actor = new St.Bin({ name: 'appMenu', });
>+        this._container = new Shell.GenericContainer();

the actor vs _container split would be unnecessary if GenericContainer was ported to St, right? We really ought to do that at some point...

appMenu has "spacing: 4px" in gnome-shell.css, which is irrelevant now and should be removed. (Actually, it was irrelevant before too...)

>+    _getPreferredWidth: function(actor, forHeight, alloc) {

"icon width/2 + label width" could theoretically be less than the width of the icon...

>+        childBox.x2 = childBox.x1 + Math.min(naturalWidth, allocWidth);
>+        childBox.y2 = childBox.y1 + Math.min(allocHeight, naturalHeight);

nitpicky, but both here and again below you swap the order of natural vs alloc in the two lines.


for extra credit, you could fix this for RTL at the same time. (It would be nice if we could keep this as an St.BoxLayout somehow... would "padding: -12px" work? Would we *want* it to work?)

(In reply to comment #5)
> (From update of attachment 154911 [details] [review])
> >+        this.actor = new St.Bin({ name: 'appMenu', });

er, fix the trailing comma too. (squash that fix from the second patch)

> appMenu has "spacing: 4px" in gnome-shell.css, which is irrelevant now and
> should be removed. (Actually, it was irrelevant before too...)

there's also

  .app-menu-icon {
      width: 24px;
      height: 24px;
  }

which I mistakenly assumed was doing something, but I don't think it is

Comment on attachment 154912 [details] [review]
[panel] Handle async load of icons correctly

>-            let faded = Shell.fade_app_icon(icon); /* TODO consider caching */

the comment still applies doesn't it? we re-fade the icon each time you switch to the app.

>+                faded = new Clutter.Texture({ width: AppDisplay.APPICON_SIZE,
>+                                              height: AppDisplay.APPICON_SIZE });

could that be this._sourceIcon.width and this._sourceIcon.height? that will save us some search-and-replacing when we move or CSS-ify appIcon.

Created attachment 154956 [details] [review]
[panel] Fix RTL for appMenu, allocation, other misc.

Review of attachment 154913 [details] [review]:

This works.

Comment on attachment 154956 [details] [review]
[panel] Fix RTL for appMenu, allocation, other misc.

>+        if (direction == St.TextDirection.LTR) {
>+            childBox.x1 = Math.floor(childBox.x2 / 2);  // Pull in width of iconBox
>+            childBox.x2 = childBox.x1 + Math.min(naturalWidth, allocWidth);
>+        } else {
>+            childBox.x1 = Math.max(0, allocWidth - Math.floor(childBox.x2 / 2) - naturalWidth);
>+            childBox.x2 = allocWidth;
>+        }
>         this._label.actor.allocate(childBox, flags);

If the label is longer than the allocation, then x2 is too large in both cases; in the LTR case the label would extend off the right end of the container by half the width of the icon, and in the RTL case it would extend through the entire icon instead of only half of it. I think you want:

    let iconWidth = childBox.x2 - childBox.x1;
    if (direction == St.TextDirection.LTR) {
        childBox.x1 = Math.floor(iconWidth / 2);
        childBox.x2 = Math.min(childBox.x1 + naturalWidth, allocWidth);
    } else {
        childBox.x2 = allocWidth - iconWidth;
        childBox.x1 = Math.max(0, childBox.x2 - naturalWidth);
    }

Comment on attachment 154913 [details] [review]
[windowTracker] Fix tracking of single-process multi app cases

Pushed as 40a8e9c

Comment on attachment 154956 [details] [review]
[panel] Fix RTL for appMenu, allocation, other misc.

Attachment 154956 [details] pushed as 347196d - [panel] Fix RTL for appMenu, allocation, other misc.