GNOME Bugzilla – Bug 602131
Port AppWell to CSS
Last modified: 2009-11-23 19:35:49 UTC
And other associated patches.
Created attachment 147931 [details] [review] [ShellDrawingArea] Add _queue_redraw method If a drawing parameter other than the allocation changed, call this method.
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.
Created attachment 147933 [details] [review] [StClickable] Import ShellButtonBox as StClickable Now a StBin, and add hover/active style properties. Otherwise a straightforward namespace transformation.
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.
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 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 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 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 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
(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 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
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.
Created attachment 148008 [details] [review] [StClickable] Import ShellButtonBox as StClickable Now a StBin, and add hover/active style properties. Otherwise a straightforward namespace transformation.
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.
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.
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.
Created attachment 148012 [details] [review] [panel] Port to CSS
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 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 on attachment 148010 [details] [review] Port AppWell, altTab to CSS; delete appIcon.js likewise afaict this is mostly identical to the earlier version
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 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 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 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?
(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 =)
(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.
(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.
(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
(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.
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;
Created attachment 148144 [details] [review] [dash] Switch to using StWidget gradients
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.
Created attachment 148146 [details] [review] Import ShellButtonBox as StClickable Now a StBin, and add hover/active style properties. Otherwise a straightforward namespace transformation.
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.
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.
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.
Created attachment 148150 [details] [review] [panel] Port to CSS
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 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 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.)
Attachment 148143 [details] pushed as 7b9f5b7 - Implement gradients for StWidget Attachment 148144 [details] pushed as 8c59bc7 - [dash] Switch to using StWidget gradients
Created attachment 148242 [details] [review] Remove information icon It wasn't really useful, not obviously in the current mockups, and depended on button.js.
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 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 on attachment 148183 [details] [review] Move ShellDrawingArea to StDrawingArea afaict this still does not emit redraw on style_set
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 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.
(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.
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