After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 602131 - Port AppWell to CSS
Port AppWell to CSS
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2009-11-16 19:30 UTC by Colin Walters
Modified: 2009-11-23 19:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[ShellDrawingArea] Add _queue_redraw method (1.52 KB, patch)
2009-11-16 19:30 UTC, Colin Walters
accepted-commit_now Details | Review
[StThemeNode] Re-add erroneously deleted (out) annotations (1.76 KB, patch)
2009-11-16 19:30 UTC, Colin Walters
committed Details | Review
[StClickable] Import ShellButtonBox as StClickable (11.76 KB, patch)
2009-11-16 19:30 UTC, Colin Walters
reviewed Details | Review
Remove ShellButtonBox, button.js; Use St.Clickable in panel.js (21.88 KB, patch)
2009-11-16 19:30 UTC, Colin Walters
needs-work Details | Review
Port AppWell, altTab to CSS; delete appIcon.js (64.87 KB, patch)
2009-11-16 19:30 UTC, Colin Walters
reviewed Details | Review
Move ShellDrawingArea to StDrawingArea, implement gradients for StWidget (43.53 KB, patch)
2009-11-18 00:02 UTC, Colin Walters
reviewed Details | Review
[StClickable] Import ShellButtonBox as StClickable (11.81 KB, patch)
2009-11-18 00:02 UTC, Colin Walters
reviewed Details | Review
Remove ShellButtonBox, button.js; Use St.Clickable in panel.js (21.88 KB, patch)
2009-11-18 00:02 UTC, Colin Walters
reviewed Details | Review
Port AppWell, altTab to CSS; delete appIcon.js (53.29 KB, patch)
2009-11-18 00:02 UTC, Colin Walters
reviewed Details | Review
[genericDisplay] Port information icon to CSS (4.63 KB, patch)
2009-11-18 00:02 UTC, Colin Walters
reviewed Details | Review
[panel] Port to CSS (11.94 KB, patch)
2009-11-18 00:02 UTC, Colin Walters
reviewed Details | Review
Implement gradients for StWidget (13.49 KB, patch)
2009-11-19 23:30 UTC, Colin Walters
committed Details | Review
[dash] Switch to using StWidget gradients (3.08 KB, patch)
2009-11-19 23:30 UTC, Colin Walters
committed Details | Review
Move ShellDrawingArea to StDrawingArea (12.53 KB, patch)
2009-11-19 23:31 UTC, Colin Walters
none Details | Review
Import ShellButtonBox as StClickable (11.83 KB, patch)
2009-11-19 23:31 UTC, Colin Walters
committed Details | Review
Remove ShellButtonBox, button.js; Use St.Clickable in panel.js (23.39 KB, patch)
2009-11-19 23:31 UTC, Colin Walters
committed Details | Review
Port AppWell to CSS; delete appIcon.js (42.54 KB, patch)
2009-11-19 23:31 UTC, Colin Walters
committed Details | Review
Port information icon to CSS (4.61 KB, patch)
2009-11-19 23:31 UTC, Colin Walters
none Details | Review
[panel] Port to CSS (10.39 KB, patch)
2009-11-19 23:31 UTC, Colin Walters
none Details | Review
Move ShellDrawingArea to StDrawingArea (17.98 KB, patch)
2009-11-20 18:13 UTC, Colin Walters
committed Details | Review
Remove information icon (10.15 KB, patch)
2009-11-21 20:47 UTC, Colin Walters
committed Details | Review
[panel] Port to CSS (15.21 KB, patch)
2009-11-21 20:47 UTC, Colin Walters
committed Details | Review

Description Colin Walters 2009-11-16 19:30:24 UTC
And other associated patches.
Comment 1 Colin Walters 2009-11-16 19:30:27 UTC
Created attachment 147931 [details] [review]
[ShellDrawingArea] Add _queue_redraw method

If a drawing parameter other than the allocation changed, call
this method.
Comment 2 Colin Walters 2009-11-16 19:30:30 UTC
Created attachment 147932 [details] [review]
[StThemeNode] Re-add erroneously deleted (out) annotations

An earlier commit was overzealous in removing (out) annotations;
introspection supports (out) for integral types just fine, we
only need to remove them for (out) types where the caller needs
to allocate a boxed type.
Comment 3 Colin Walters 2009-11-16 19:30:33 UTC
Created attachment 147933 [details] [review]
[StClickable] Import ShellButtonBox as StClickable

Now a StBin, and add hover/active style properties.  Otherwise a
straightforward namespace transformation.
Comment 4 Colin Walters 2009-11-16 19:30:36 UTC
Created attachment 147934 [details] [review]
Remove ShellButtonBox, button.js; Use St.Clickable in panel.js

StClickable replaces ShellButtonBox.  The only user remaining of
ShellButtonBox was button.js, which itself was only used from
panel.js.  Delete both of them, and just use St.Clickable in
panel.  To do so, add CSS style for the actitivies button.
Comment 5 Colin Walters 2009-11-16 19:30:39 UTC
Created attachment 147935 [details] [review]
Port AppWell, altTab to CSS; delete appIcon.js

The altTab.js and app well code weren't sharing really
any functionality anymore; un-merge the appIcon code back
into appWell, and have a simple icon + text display for
altTab.

Port AppWell to St and CSS.
Comment 6 Colin Walters 2009-11-16 20:12:13 UTC
Comment on attachment 147934 [details] [review]
Remove ShellButtonBox, button.js; Use St.Clickable in panel.js

Hm, somehow missed a few other uses.  Updated patch inc
Comment 7 Dan Winship 2009-11-16 20:26:16 UTC
Comment on attachment 147933 [details] [review]
[StClickable] Import ShellButtonBox as StClickable

>+ * A #StBin subclass which translates lower-level Clutter button eve

end of the line got cut off

>+G_DEFINE_TYPE(StClickable, st_clickable, ST_TYPE_BIN);

space before (

>+set_active (StClickable  *box,

you might want to s/box/clickable/ in all the argument names

also, you need a call to sync_pseudo_class() in set_active().

>+st_clickable_leave_event (ClutterActor         *actor,
>+                              ClutterCrossingEvent *event)

needs reindenting

>+ * If this button box is holding a pointer grab, this function will

s/button box/clickable/ or /widget/

>+st_clickable_set_property(GObject         *object,
>+                              guint            prop_id,

space before (, indentation

>+   * This signal is emitted when the button should take the action
>+   * associated with button click+release.

s/button/widget/ ?

>+   * StClickable:active
>+   *
>+   * The property allows the button to be used as a "toggle button"; it's up to the
>+   * application to update the active property in response to the activate signal;

there's no "activate" signal
Comment 8 Dan Winship 2009-11-16 21:43:31 UTC
Comment on attachment 147935 [details] [review]
Port AppWell, altTab to CSS; delete appIcon.js

>Subject: [PATCH] Port AppWell, altTab to CSS; delete appIcon.js

suggests you're porting altTab to CSS, which you aren't really

>+++ b/js/ui/Makefile.am
>@@ -4,7 +4,7 @@ dist_jsui_DATA =		\
> 	altTab.js		\
> 	appDisplay.js		\
> 	appFavorites.js		\
>-    calendar.js		\
>+    calendar.js     \

accident?

>+        let iconBox = new St.Bin({ width: APPICON_SIZE,
>+                                   height: APPICON_SIZE });
>+        this.icon = this.app.create_icon_texture(APPICON_SIZE);
>+        iconBox.set_child(this.icon);

does iconBox really do anything? Couldn't you just set .width and .height on this.icon?

>+        } else if (button == 3) {
>+            this.popupMenu(0);

why not this.popupMenu(button)? Or with no arg at all; assuming we do want to pass "0" to this._menu.popup() later on, should we just hardcode the "0" there rather than here?

>+                    // The button won't get any leave events while
>+                    // the menu is up, so reset the style here
>+                    this.actor.set_style_pseudo_class(null);

That's an abstraction violation now (and it will leave the clickable's "pressed" and "hover" properties with the wrong values won't it?)

>+    _appendMenuItem: function(labelText) {
>+        /* Use padding here rather than spacing in the box above so that
>+         * we have a larger reactive area.
>+         */

this comment is no longer relevant to the code around it
Comment 9 Dan Winship 2009-11-16 21:44:14 UTC
Comment on attachment 147931 [details] [review]
[ShellDrawingArea] Add _queue_redraw method

ok, although another possibility would be to port this to StDrawingArea and then just have it automatically do a redraw on style-set
Comment 10 Colin Walters 2009-11-16 21:45:27 UTC
(In reply to comment #9)
> (From update of attachment 147931 [details] [review])
> ok, although another possibility would be to port this to StDrawingArea and
> then just have it automatically do a redraw on style-set

I like that idea, will do an updated patch.
Comment 11 Colin Walters 2009-11-17 23:44:41 UTC
Comment on attachment 147932 [details] [review]
[StThemeNode] Re-add erroneously deleted (out) annotations

Attachment 147932 [details] pushed as 8ac97fe - [StThemeNode] Re-add erroneously deleted (out) annotations
Comment 12 Colin Walters 2009-11-18 00:02:20 UTC
Created attachment 148007 [details] [review]
Move ShellDrawingArea to StDrawingArea, implement gradients for StWidget

It's nicer to have ShellDrawingArea as a St widget so it can
participate more cleanly in CSS styling, such as queuing a redraw
automatically on style changes, and allowing subclasses to use
CSS styling.

Next, rather than having gradients be individually implemented by higher
level JS widgets, move basic gradient functionality into StWidget.
There is prior art in WebKit for CSS gradients:

http://webkit.org/blog/175/introducing-css-gradients/

However, implementing this would be quite a lot of work; all we
need in the Shell design at the moment is basic horizontal/vertical
linear gradients.  So, the syntax now supported is:

background-gradient-type: [vertical|horizontal]
background-gradient-start: color;
background-gradient-end: color;

Note StWidget uses StDrawingArea now to implement the gradients.
Comment 13 Colin Walters 2009-11-18 00:02:28 UTC
Created attachment 148008 [details] [review]
[StClickable] Import ShellButtonBox as StClickable

Now a StBin, and add hover/active style properties.  Otherwise a
straightforward namespace transformation.
Comment 14 Colin Walters 2009-11-18 00:02:35 UTC
Created attachment 148009 [details] [review]
Remove ShellButtonBox, button.js; Use St.Clickable in panel.js

StClickable replaces ShellButtonBox.  The only user remaining of
ShellButtonBox was button.js, which itself was only used from
panel.js.  Delete both of them, and just use St.Clickable in
panel.  To do so, add CSS style for the actitivies button.
Comment 15 Colin Walters 2009-11-18 00:02:42 UTC
Created attachment 148010 [details] [review]
Port AppWell, altTab to CSS; delete appIcon.js

The altTab.js and app well code weren't sharing really
any functionality anymore; un-merge the appIcon code back
into appWell, and have a simple icon + text display for
altTab.

Port AppWell to St and CSS.
Comment 16 Colin Walters 2009-11-18 00:02:47 UTC
Created attachment 148011 [details] [review]
[genericDisplay] Port information icon to CSS

The information icon got readded after an earlier CSS port, port
it now since it depends on Button.
Comment 17 Colin Walters 2009-11-18 00:02:53 UTC
Created attachment 148012 [details] [review]
[panel] Port to CSS
Comment 18 Dan Winship 2009-11-18 22:25:38 UTC
Comment on attachment 148007 [details] [review]
Move ShellDrawingArea to StDrawingArea, implement gradients for StWidget

>Subject: [PATCH] Move ShellDrawingArea to StDrawingArea, implement gradients for StWidget

would be nicer to have those as separate patches

>+function AppIconMenu(source) {
>+    this._init(source);
>+}
>+...

rebase fail? Doesn't belong in this patch.

>+ * clutter_cairo_texture_create() to begin drawing.  The
>+ * @redraw signal will be emitted by default when the area is
>+ * resized or the CSS style changes; you can use the

You aren't connecting to style_changed though. It looks to me like redraw will only be automatically emitted if the size changes or if the border/padding widths change.

Correct gtk-doc way to refer to a signal to get it linkified is "#StDrawingArea::redraw".

>+  if (width > 0 && height > 0)
>+    {
>+      clutter_cairo_texture_set_surface_size (area->priv->texture,
>+                                              width, height);

hm... should it be adjusted for borders/padding?

and if width or height==0 don't you need to shrink/hide the cairo texture?

>+/**
>+ * st_theme_node_get_background_gradient:
>+ * @node: A #StThemeNode
>+ * @vertical: (out): %TRUE iff the gradient is vertical, otherwise horizontal

doc is wrong (@vertical should be @type)

>+ * @start: Color at top of gradient
>+ * @end: Color at bottom of gradient

"top" "bottom" are only if vertical

>-    }
>+      }
>+    else if (priv->bg_gradient_type != ST_GRADIENT_NONE)

this entire clause seems to be indented two spaces too far

>+        ClutterActorBox frame_box = {
>+          0, 0, box->x2 - box->x1, box->y2 - box->y1
>+        };

C89 requires struct initializer values to be compile-time constants, if we care. (Is suncc C99 yet?)

>+  st_theme_node_get_background_gradient (theme_node, &gradient, &color, &gradient_end);
>+
>+  if (gradient == ST_GRADIENT_NONE)
>+    {
>+      st_theme_node_get_background_color (theme_node, &color);

last line is redundant; the call to _get_background_gradient already loaded the bg color into &color.

>+  else if (gradient != priv->bg_gradient_type)

|| !clutter_color_equal (&gradient_end, &priv->bg_gradient_end)
Comment 19 Dan Winship 2009-11-19 15:42:37 UTC
Comment on attachment 148008 [details] [review]
[StClickable] Import ShellButtonBox as StClickable

afaict you did not address any of the things I commented on before except for adding the missing sync_pseudo_class().

(which I guess means that the code is now all correct afaict, but the style and comments are still wrong in places)
Comment 20 Dan Winship 2009-11-19 15:42:56 UTC
Comment on attachment 148010 [details] [review]
Port AppWell, altTab to CSS; delete appIcon.js

likewise afaict this is mostly identical to the earlier version
Comment 21 Dan Winship 2009-11-19 15:53:19 UTC
Comment on attachment 148009 [details] [review]
Remove ShellButtonBox, button.js; Use St.Clickable in panel.js

>-	calendar.js		\
>+    calendar.js		\

Makefiles should always use tabs not spaces

>-        this.button = new Button.Button(_("Activities"), PANEL_BUTTON_COLOR, PRESSED_BUTTON_BACKGROUND_COLOR,

I believe those two colors are now unused and can be deleted.

>+        this.button.connect('clicked', Lang.bind(this, function(b) {
>+            let event = Clutter.get_current_event();

event is unused
Comment 22 Dan Winship 2009-11-19 15:55:23 UTC
Comment on attachment 148009 [details] [review]
Remove ShellButtonBox, button.js; Use St.Clickable in panel.js

panel.js uses Button.Button for the status menu too, but this patch doesn't seem to change that?
Comment 23 Dan Winship 2009-11-19 17:22:11 UTC
Comment on attachment 148011 [details] [review]
[genericDisplay] Port information icon to CSS

looks good, but is buttonBox really necessary? it seems like we ought to be able to set padding or border or margin or something on the St.Clickable itself to get the same effect?
Comment 24 Dan Winship 2009-11-19 17:38:49 UTC
Comment on attachment 148012 [details] [review]
[panel] Port to CSS

>+.panel-button:pressed {
>     background-color: rgba(50,76,111,0.98);
>+    border-radius: 4px;
>+}

hm... the decreased border-radius is to create a pressed-in effect? I think you probably want to shift the content by 1 or 2 pixels (eg, increase top+left padding, decrease bottom+right padding).

>         let bin = new St.Bin({ style_class: 'item-box' });
>-        let bbox = new Shell.ButtonBox({ reactive: true });
>+        let bbox = new St.Clickable({ reactive: true,

as in the previous patch, it might be possible to get rid of the St.Bin and just use appropriate CSS styling to get the same effect.

>diff --git a/js/ui/dash.js b/js/ui/dash.js
>-const Button = imports.ui.button;

presumably actually belongs in the patch that got rid of button.js

>-        this._iconBox.remove_all();
>+        if (this._iconBox.get_child())
>+            this._iconBox.get_child().destroy();

St.Bin has a "child" property, so you can say just "this._iconBox.child".

(You can also just do "this._iconBox.child = null;", although that won't cause it to be destroy()ed until it gets GCed I guess...)

>+            this._iconBox.set_child(icon);

likewise can do "this._iconBox.child = icon;" (and the same in other places where you use set_child()).

>+        statusbutton.height = PANEL_HEIGHT;

could have been specifi

>+        statusbutton.connect('clicked', function (b) {
>+            let event = Clutter.get_current_event();
>+            statusmenu.toggle(event);

this seems a little odd. Shouldn't St.Clickable just include the event in the signal args?
Comment 25 Colin Walters 2009-11-19 21:19:24 UTC
(In reply to comment #8)
> 
> >+        } else if (button == 3) {
> >+            this.popupMenu(0);
> 
> why not this.popupMenu(button)? Or with no arg at all; assuming we do want to
> pass "0" to this._menu.popup() later on, should we just hardcode the "0" there
> rather than here?

This is on a click/release; the 0 means to allow deactivation with any button.

> >+                    // The button won't get any leave events while
> >+                    // the menu is up, so reset the style here
> >+                    this.actor.set_style_pseudo_class(null);
> 
> That's an abstraction violation now (and it will leave the clickable's
> "pressed" and "hover" properties with the wrong values won't it?)

Well we know it's not pressed any more.  The basic problem here is that we're doing a pointer grab in ShellMenu, which means the button won't get notification of leave so won't know to reset its hover state.  

It looks like the MxPopup class doesn't do a pointer grab at all, instead does event capturing, but it takes MxAction and doesn't support arbitrary widgets as menuitems, so would not be a trivial adaption.

Not quite sure what to do here; I could try to rewrite Shell.Menu in this way, but I'd like to at some point move on to code that isn't just under-the-covers changes =)
Comment 26 Colin Walters 2009-11-19 22:51:28 UTC
(In reply to comment #18)

> and if width or height==0 don't you need to shrink/hide the cairo texture?

In that case it's hidden and won't be painted anyways, I'm pretty sure.

> last line is redundant; the call to _get_background_gradient already loaded the
> bg color into &color.

Hmm, I don't want to rely on that in this code.  The contract is if type == ST_GRADIENT_NONE, the latter two values aren't necessarily set.  I changed the _get_gradient code to do that explicitly.
Comment 27 Colin Walters 2009-11-19 22:59:53 UTC
(In reply to comment #23)
> (From update of attachment 148011 [details] [review])
> looks good, but is buttonBox really necessary? it seems like we ought to be
> able to set padding or border or margin or something on the St.Clickable itself
> to get the same effect?

The width/height properties include the padding and border, and thus the icon would be incorrectly sized.
Comment 28 Colin Walters 2009-11-19 23:27:49 UTC
(In reply to comment #24)
> (From update of attachment 148012 [details] [review])
> >+.panel-button:pressed {
> >     background-color: rgba(50,76,111,0.98);
> >+    border-radius: 4px;
> >+}
> 
> hm... the decreased border-radius is to create a pressed-in effect? I think you
> probably want to shift the content by 1 or 2 pixels (eg, increase top+left
> padding, decrease bottom+right padding).

Hm?  It's not decreased; I guess I could move border-radius to apply always, but it would be invisible since the background is transparent right now.


> >diff --git a/js/ui/dash.js b/js/ui/dash.js
> >-const Button = imports.ui.button;
> 
> presumably actually belongs in the patch that got rid of button.js

Yeah...these patches are not fully independent; the lower ones in the stack are, but switching to Clickable is intertwined with porting things to CSS.


> likewise can do "this._iconBox.child = icon;" (and the same in other places
> where you use set_child()).

Hmm, I like using the method call for important actions like this.
 
> >+        statusbutton.height = PANEL_HEIGHT;
> 
> could have been specifi


Truncated?

> >+        statusbutton.connect('clicked', function (b) {
> >+            let event = Clutter.get_current_event();
> >+            statusmenu.toggle(event);
> 
> this seems a little odd. Shouldn't St.Clickable just include the event in the
> signal args?

Well, there are other possible places where we would want this, and it's annoying to have to emit the event as an argument for every signal.  GTK+ has a similar API.

Actually it is important to note here that this patch depends on a clutter patch to add thsi function:

http://bugzilla.o-hand.com/show_bug.cgi?id=1888
Comment 29 Colin Walters 2009-11-19 23:29:06 UTC
(In reply to comment #28)
>
> Yeah...these patches are not fully independent; the lower ones in the stack
> are, but switching to Clickable is intertwined with porting things to CSS.

Note once the St layer settles down a bit, it should reduce the interdependence of patches.
Comment 30 Colin Walters 2009-11-19 23:30:46 UTC
Created attachment 148143 [details] [review]
Implement gradients for StWidget

Rather than having gradients be individually implemented by higher
level JS widgets, move basic gradient functionality into StWidget.
There is prior art in WebKit for CSS gradients:

http://webkit.org/blog/175/introducing-css-gradients/

However, implementing this would be quite a lot of work; all we
need in the Shell design at the moment is basic horizontal/vertical
linear gradients.  So, the syntax now supported is:

background-gradient-type: [vertical|horizontal]
background-gradient-start: color;
background-gradient-end: color;
Comment 31 Colin Walters 2009-11-19 23:30:53 UTC
Created attachment 148144 [details] [review]
[dash] Switch to using StWidget gradients
Comment 32 Colin Walters 2009-11-19 23:31:06 UTC
Created attachment 148145 [details] [review]
Move ShellDrawingArea to StDrawingArea

It's nicer to have ShellDrawingArea as a St widget so it can
participate more cleanly in CSS styling, such as queuing a redraw
automatically on style changes, and allowing subclasses to use
CSS styling.
Comment 33 Colin Walters 2009-11-19 23:31:15 UTC
Created attachment 148146 [details] [review]
Import ShellButtonBox as StClickable

Now a StBin, and add hover/active style properties.  Otherwise a
straightforward namespace transformation.
Comment 34 Colin Walters 2009-11-19 23:31:23 UTC
Created attachment 148147 [details] [review]
Remove ShellButtonBox, button.js; Use St.Clickable in panel.js

StClickable replaces ShellButtonBox.  The only user remaining of
ShellButtonBox was button.js, which itself was only used from
panel.js.  Delete both of them, and just use St.Clickable in
panel.  To do so, add CSS style for the actitivies button.
Comment 35 Colin Walters 2009-11-19 23:31:33 UTC
Created attachment 148148 [details] [review]
Port AppWell to CSS; delete appIcon.js

The altTab.js and app well code weren't sharing really
any functionality anymore; un-merge the appIcon code back
into appWell, and have a simple icon + text display for
altTab.

Port AppWell to St and CSS.
Comment 36 Colin Walters 2009-11-19 23:31:43 UTC
Created attachment 148149 [details] [review]
Port information icon to CSS

The information icon got readded after an earlier CSS port, port
it now since it depends on Button.
Comment 37 Colin Walters 2009-11-19 23:31:50 UTC
Created attachment 148150 [details] [review]
[panel] Port to CSS
Comment 38 Colin Walters 2009-11-20 18:13:26 UTC
Created attachment 148183 [details] [review]
Move ShellDrawingArea to StDrawingArea

It's nicer to have ShellDrawingArea as a St widget so it can
participate more cleanly in CSS styling, such as queuing a redraw
automatically on style changes, and allowing subclasses to use
CSS styling.
Comment 39 Dan Winship 2009-11-20 23:49:06 UTC
Comment on attachment 148143 [details] [review]
Implement gradients for StWidget

>+          else if (strcmp (term->content.str->stryng->str, "horizontal") == 0)
>+            {
>+               node->background_gradient_type = ST_GRADIENT_HORIZONTAL;

this line and the g_warning in the following "else" are indented one space too far

>+      if (width > 0 && height > 0)
>+      clutter_cairo_texture_set_surface_size (CLUTTER_CAIRO_TEXTURE (priv->background_image),

and this line is indented too little

>+  if (gradient == ST_GRADIENT_NONE)

pretty sure you need to be setting priv->bg_gradient_type = ST_GRADIENT_NONE in this case.

otherwise looks good to commit
Comment 40 Dan Winship 2009-11-21 00:01:05 UTC
Comment on attachment 148147 [details] [review]
Remove ShellButtonBox, button.js; Use St.Clickable in panel.js

> StClickable replaces ShellButtonBox.  The only user remaining of
> ShellButtonBox was button.js

and altTab.js...

>-	appIcon.js		\

should go in the appicon patch

>diff --git a/js/ui/altTab.js b/js/ui/altTab.js
>         // bin inside the ButtonBox rather than vice versa.

s/ButtonBox/Clickable in that comment. (I don't think it's really worth renaming the bbox variable.)
Comment 41 Colin Walters 2009-11-21 00:52:53 UTC
Attachment 148143 [details] pushed as 7b9f5b7 - Implement gradients for StWidget
Attachment 148144 [details] pushed as 8c59bc7 - [dash] Switch to using StWidget gradients
Comment 42 Colin Walters 2009-11-21 20:47:34 UTC
Created attachment 148242 [details] [review]
Remove information icon

It wasn't really useful, not obviously in the current mockups, and
depended on button.js.
Comment 43 Colin Walters 2009-11-21 20:47:44 UTC
Created attachment 148243 [details] [review]
[panel] Port to CSS

With some tweaks from JP St. Pierre to center things in case we ever
have a larger panel.
Comment 44 Dan Winship 2009-11-21 23:51:34 UTC
Comment on attachment 148148 [details] [review]
Port AppWell to CSS; delete appIcon.js

>Subject: [PATCH] Port AppWell to CSS; delete appIcon.js

this might be one of the things you mentioned on irc yesterday, but this patch does not actually include the removal of appIcon.js (or its removal from Makefile.am).

Also, comparing to the earlier version of this patch, the big change seems to be that you've gotten rid of some of the excess containers that were only there to add padding, and yet you don't seem to have changed the CSS at all, so I can't see how that would work. Did the CSS changes end up in the wrong patch maybe?

Likewise, from my earlier review:

>>+                    this.actor.set_style_pseudo_class(null);
>
>That's an abstraction violation now (and it will leave the clickable's
>"pressed" and "hover" properties with the wrong values won't it?)

The line I complained about is now gone, and yet I can't see any changes to StClickable or anywhere else to replace it. Or did that turn out to just be unnecessary?
Comment 45 Dan Winship 2009-11-21 23:57:32 UTC
Comment on attachment 148183 [details] [review]
Move ShellDrawingArea to StDrawingArea

afaict this still does not emit redraw on style_set
Comment 46 Dan Winship 2009-11-22 00:01:08 UTC
Comment on attachment 148242 [details] [review]
Remove information icon

The additions to gnome-shell.css look like leftovers from an earlier attempt to port the info icon to CSS rather than removing it, and presumably should be reverted? Otherwise good
Comment 47 Dan Winship 2009-11-22 00:10:08 UTC
Comment on attachment 148243 [details] [review]
[panel] Port to CSS

looks good.

> > >+        statusbutton.height = PANEL_HEIGHT;
> > 
> > could have been specifi
> 
> Truncated?

I was going to say "could have been specified as a constructor property", but then I was like "eh, whatever", and meant to delete the comment but I apparently failed.
Comment 48 Colin Walters 2009-11-23 19:16:00 UTC
(In reply to comment #44)
> (From update of attachment 148148 [details] [review])
> >Subject: [PATCH] Port AppWell to CSS; delete appIcon.js
> 
> this might be one of the things you mentioned on irc yesterday, but this patch
> does not actually include the removal of appIcon.js (or its removal from
> Makefile.am).

Hmm it's clearly here locally.  Not sure what happened; pilot error with git-bz?   I can reattach the one I have locally.

> Also, comparing to the earlier version of this patch, the big change seems to
> be that you've gotten rid of some of the excess containers that were only there
> to add padding, and yet you don't seem to have changed the CSS at all, so I
> can't see how that would work. Did the CSS changes end up in the wrong patch
> maybe?

Hm, looking at the interdiff it appears to be the other way around; I only see changes to the CSS basically.

> Likewise, from my earlier review:
> 
> >>+                    this.actor.set_style_pseudo_class(null);
> >
> >That's an abstraction violation now (and it will leave the clickable's
> >"pressed" and "hover" properties with the wrong values won't it?)
> 
> The line I complained about is now gone, and yet I can't see any changes to
> StClickable or anywhere else to replace it. Or did that turn out to just be
> unnecessary?

I decided to leave it slightly buggy for now until I figure out some saner solution, which may involve porting things to StPopup.
Comment 49 Colin Walters 2009-11-23 19:35:22 UTC
Attachment 148146 [details] pushed as 93f3412 - Import ShellButtonBox as StClickable
Attachment 148147 [details] pushed as 2f1ca7b - Remove ShellButtonBox, button.js; Use St.Clickable in panel.js
Attachment 148148 [details] pushed as 907fc2f - Port AppWell to CSS; delete appIcon.js
Attachment 148183 [details] pushed as f815844 - Move ShellDrawingArea to StDrawingArea
Attachment 148242 [details] pushed as 73cd951 - Remove information icon
Attachment 148243 [details] pushed as 1d2dc09 - [panel] Port to CSS