Created attachment 181249 [details] [review]
panel: Move statusmenu initialization in the constructor

While related to the status area, the user status button is clearly
not a status icon, and it does not make too much sense in
startStatusArea(), which is about filling the status area with
icons. Also, the status icon container is added to the panel in
the constructor, in fact, the user status button is the only "toplevel"
panel element which is initialized elsewhere. Not a crucial change,
but makes for a nice read anyway.

Created attachment 181250 [details] [review]
panel: Update style

Update the style of the panel to match the latest mockups.

 - add a subtle border at the bottom of the panel
 - use a highlight of active panel buttons similar to the one
   used for running applications in the dash/app view
 - shadow the buttons' label/icon when highlighted

Created attachment 181251 [details] [review]
panel: Add rounded corners

Current mockups show the panel curving downwards at the edges to
frame the work area and look awesome. Implement those as separate
actors to not affect the struts set by the panel, and synchronize
their state with the corresponding panel buttons so they blend in
with the panel. It might be worth considering whether the corners
should be hidden with maximized windows on the current workspace,
though this might affect the illusion of them being part of the
panel. As the corners don't affect the input region, the small
overlap with windows might not be too bad after all.

Created attachment 181252 [details] [review]
panel-buttons: Remove transitions for corner buttons

So far transitions do not work for the custom drawn corners, so to
avoid a visible glitch when transitioning a button in the panel corner
while updating the style of the apparently attached corner instantly,
remove transition of those panel buttons until we make it work for the
custom drawn parts as well.

Created attachment 181253 [details] [review]
calendar: Update style

The style used to highlight today's date should follow the style
of active panel buttons, so update it to reflect the new panel
style.

*** Bug 614507 has been marked as a duplicate of this bug. ***

Review of attachment 181249 [details] [review]:

Fine

Review of attachment 181250 [details] [review]:

There's a clear visual problem here compared to the mockups that in the mockups the big bold white line is under only the button content while in this patch the big bold line extends out under the padding as well. Maybe we should wait for review comments from jimmac, but it definitely looks odd to me. Two possible fixes:

 - Add an intermediate container inside panel buttons that wraps the content tightly in the horizontal direction and add the border to that
 - use a border image which has gaps to match the 12px padding on panel buttons

Thre's also a problem that we've grown a two-pixel gap under the app menu. This might encourage something like the second approach - more flexibility in how we pack things. Other than these visual problems patch looks fine

(In reply to comment #8)
> There's a clear visual problem here compared to the mockups that in the mockups
> the big bold white line is under only the button content while in this patch
> the big bold line extends out under the padding as well.

Yeah, that's what I meant in the original comment.


>  - use a border image which has gaps to match the 12px padding on panel buttons

Dito.

Created attachment 181289 [details] [review]
panel: Update style

Use a border-image for active panel buttons.

Created attachment 181290 [details] [review]
appMenu: Clip app icon when the button is active

We now use a border image on active panel buttons to underline the
button's content. As the property does not affect the content's
allocation, the app icon ends up being drawn on top of the border
image. To prevent this, use a custom property to clip the bottom of
the app icon when the button is active.

Created attachment 181291 [details] [review]
calendar: Update style

Forgot to add calendar-today.svg to Makefile.am.

Review of attachment 181251 [details] [review]:

Looks reasonable when tried out, and the general code structure seems fine. Various comments (mostly stylistic) about the details of the drawing.

::: js/ui/panel.js
@@ +62,3 @@
+// from st-theme-node-drawing.c
+function _norm(x) {
+    return ((x + 127.) + (x + 127.) / 255.) / 255.;

Hmm, the original is an integer replacement of floating point algorithm to avoid division - it doesn't really make sense to try and do that as floating point.

http://people.redhat.com/otaylor/pixel-converting.html

explains where the expression comes from - it's actually equivalent to:

 Math.floor(x / 255 + 0.5)  == Math.round(x / 255);

@@ +77,3 @@
+
+    src._unpremultiply();
+    dst._unpremultiply();

Don't like modifing and then unmodifying the input, better if premultiply and unpremultiply just allocated new objects I think.

@@ +83,3 @@
+}
+
+Clutter.Color.prototype._premultiply = function() {

Don't like doing this - would rather you just had

 function _premultiply(color) {
    return Clutter.Color({ red: _norm(...),
                                       green: ... })

@@ +550,3 @@
+        this.actor = new St.DrawingArea({ style_class: 'panel-corner' });
+        this.actor.connect('repaint', Lang.bind(this, this._repaint));
+        this.actor.connect('style-changed', Lang.bind(this, this._reposition));

Bit ugly, but can't think of any particular harm from doing this... should work well enough.

@@ +568,3 @@
+
+        if (this._side == St.Side.LEFT) {
+            cr.lineTo(0, innerBorderWidth + outerBorderWidth);

It's weird to use a cr.lineTo() without an initial moveTo. I guess the initial point is at 0,0 but it would be more readable to explicitly move there. Not sure I undestand the point of this point - it seems to be colinear with (0,0) and the starting point of the arc.

@@ +573,3 @@
+                   cornerRadius, Math.PI, 3 * Math.PI / 2);
+            cr.lineTo(cornerRadius, 0);
+            cr.lineTo(0, 0);

You can closePath here which is clearer than drawing back to the starting point

@@ +580,3 @@
+            cr.lineTo(cornerRadius, 0);
+            cr.lineTo(0, 0);
+            cr.lineTo(0, innerBorderWidth + outerBorderWidth + cornerRadius);

again you can cr.closePath

@@ +587,3 @@
+        cr.fill();
+
+        if (this._side == St.Side.LEFT) {

same comments as above. Also, note that the effect here really isn't as desired - if you offset a circular arc vertically, then the top part the distance between the two arcs is the offset distance, but on the side it is less and it goes to 0 at the left/right. this is actually very obvious looking at the results.

My observation here would be that since you have an implicit clip to the square bounds of the allocated area, instead of drawing exact shapes, you can draw the 3 areas as:

 - First shape you drew
 - First shape you drew, offset by (-innerBorder / sqrt(2), -innerBorder / sqrt(2))
 - First shape you drew, offset by (-((innerBorder + outerBorder) / sqrt(2), ...)

(could use a nested function for creating the path)

Review of attachment 181252 [details] [review]:

Sure. (maybe a brief comment in the CSS file about why?) Guess the simple thing that would get transitions going would be to just use images for the corner pieces rather than custom drawing. Not really sure how you'd do it otherwise.

Review of attachment 181252 [details] [review]:

This of course also produces weirdness during animation but it's hard to see full speed.

Review of attachment 181252 [details] [review]:

The last comment was actually meant to be about the patch to clip the application icon not about this patch

Review of attachment 181289 [details] [review]:

I think this looks a lot better - it feels so bad to parse an XML file to get an image of a 2-pixel rectangle, but consistency! Any particular reason you used a constant 10 pixel border than '0 10 2 10' or something? This would break if the panel was less than 20 pixels high, though, well, that's probably not going to happen.

Review of attachment 181290 [details] [review]:

Seems to work fine other than animation, which is hard to notice. Only other idea I'd have would be to use a Shell.Stack to put the border actor over the contents of the Panel.Button but likely has its own complexities.

Review of attachment 181291 [details] [review]:

Looks fine

Review of attachment 181251 [details] [review]:

Another comment here is that the corners don't correctly connect up to the underline in ar_EG.UTF-8 locale with all these patches - looks like probably a RTL issue rather than a text-width issue since it affects the user status as well as Activities but didn't really investigate.

Created attachment 181361 [details] [review]
(In reply to comment #20)

> Another comment here is that the corners don't correctly connect up to the
> underline in ar_EG.UTF-8 locale with all these patches - looks like probably a
> RTL issue rather than a text-width issue since it affects the user status as
> well as Activities but didn't really investigate.

Right, it's an(other) RTL issue - I'll attach the surprising fix (though I don't quite understand the rationale of the offending CSS, probably to provide some minimal spacing if the panel fills up (app menu label going haywire or something ...))

panel: Adjust CSS for RTL locales

The groups at the panel sides use different left/right padding, so
a slightly different CSS is required for RTL locales. Add :rtl
pseudo classes as necessary and adjust the CSS.

Created attachment 181365 [details] [review]
panel: Add rounded corners

(In reply to comment #13)
> @@ +62,3 @@
> +// from st-theme-node-drawing.c
> +function _norm(x) {
> +    return ((x + 127.) + (x + 127.) / 255.) / 255.;
>
> [...] it's actually equivalent to:
> 
>  Math.floor(x / 255 + 0.5)  == Math.round(x / 255);

OK.


> @@ +77,3 @@
> +    src._unpremultiply();
> +    dst._unpremultiply();
> 
> Don't like modifing and then unmodifying the input, better if premultiply and
> unpremultiply just allocated new objects I think.

Done.


> @@ +83,3 @@
> +}
> +
> +Clutter.Color.prototype._premultiply = function() {
> 
> would rather you just had
> 
>  function _premultiply(color) { ... }

OK, changed.


> @@ +568,3 @@
> +
> +        if (this._side == St.Side.LEFT) {
> +            cr.lineTo(0, innerBorderWidth + outerBorderWidth);
> 
> It's weird to use a cr.lineTo() without an initial moveTo.

Hopefully rewritten in a cleaner way, starting the path with moveTo() and ending with closePath().


> if you offset a circular arc vertically, then the top part the distance
> between the two arcs is the offset distance, but on the side it is less and it
> goes to 0 at the left/right. this is actually very obvious looking at the
> results.

Right.


> instead of drawing exact shapes, you can draw the 3 areas as:
> 
>  - First shape you drew
>  - First shape you drew, offset by (-innerBorder / sqrt(2), -innerBorder /
> sqrt(2))
>  - First shape you drew, offset by (-((innerBorder + outerBorder) / sqrt(2),
> ...)

OK. I'm now offsetting the original path both vertically and horizontally, but only using the border widths (e.g. no division by sqrt(2)) - if we don't scale the path to account for different radii, a slightly wrong result towards the screen edge is less disruptive than at the top, where the drawn area has to blend in with the panel/button style.

Review of attachment 181361 [details] [review]:

Looks good - might just have been an attempt to get a little extra padding between activities and the app menu

(In reply to comment #22)
> > instead of drawing exact shapes, you can draw the 3 areas as:
> > 
> >  - First shape you drew
> >  - First shape you drew, offset by (-innerBorder / sqrt(2), -innerBorder /
> > sqrt(2))
> >  - First shape you drew, offset by (-((innerBorder + outerBorder) / sqrt(2),
> > ...)
> 
> OK. I'm now offsetting the original path both vertically and horizontally, but
> only using the border widths (e.g. no division by sqrt(2)) - if we don't scale
> the path to account for different radii, a slightly wrong result towards the
> screen edge is less disruptive than at the top, where the drawn area has to
> blend in with the panel/button style.

Hmm, geometry fail on my part. Yeah,. offset by sqrt(2) just gets the thicknes right at 45 degrees and wrong at the top and bottom.

I don't if you want to fiddle with any more - if this look good, it's good enough. (Some reason, my attempt to get everything applied failed and I don't have visible corners so I can't look for myself); I think the actual right shapes aren't that hard to draw - you just need to use different radii for the three pieces.

Review of attachment 181365 [details] [review]:

marking a-c-n assuming that we think this drawing is good enough

(In reply to comment #7)
> Review of attachment 181249 [details] [review]:
> 
> Fine

No, it was not.
PopupMenuManager requires that menus are added in order, if you don't pass position. Now it thinks that the a11y menu is to the right of the status menu.

(In reply to comment #26)
> PopupMenuManager requires that menus are added in order, if you don't pass
> position. Now it thinks that the a11y menu is to the right of the status menu.

What exactly does require it / get confused about order? I checked key navigation and didn't see anything unexpected.

(In reply to comment #27)
> (In reply to comment #26)
> > PopupMenuManager requires that menus are added in order, if you don't pass
> > position. Now it thinks that the a11y menu is to the right of the status menu.
> 
> What exactly does require it / get confused about order? I checked key
> navigation and didn't see anything unexpected.

Focus the first item in the Status menu (not the menu button), then press Right.
Expected behaviour: application menu is opened.
Current behaviour: a11y menu is opened.

The bug is in the call to PopupMenuManager.addMenu you put inside the constructor, before the status area is initalized, and its menus are added to the manager.

Fixed in master.