GNOME Bugzilla – Bug 591245
style/css/nbtk
Last modified: 2009-10-01 19:33:30 UTC
I'd like to discuss design for how we style the shell in the future. A few options: * libccss ourselves * import parts of nbtk (we should kill Tidy regardless) * depend on nbtk externally * evolve a new externally visible library My take here is that we could import a few core bits of nbtk (say NbtkStylable, NbtkBin, NbtkGrid, NbtkTooltip, NbtkScrollable), kill off Tidy. Then at a later date figure out Big.Box versus Nbtk.BoxLayout, etc.
Created attachment 141205 [details] [review] Delete Tidy It wasn't used any more.
Comment on attachment 141205 [details] [review] Delete Tidy There's an import in genericDisplay.js that doesn't seem to be removed here. It would also be nice to fix the comments in altTab.js: // Icon grid. It would be nice to use Tidy.Grid for the this, // but Tidy.Grid is lame in various ways. (Eg, it seems to // have a minimum size of 200x200.) So we create a vertical // Big.Box containing multiple horizontal Big.Boxes. // TODO: Implement relative positioning using Tidy. To correspond to a "no plans to use Tidy" state. The second looks like it should be "TODO: Use Big.Box for relative positioning" The first might just need to be changed from "It would be nice to..." to "We considered..." Otherwise, looks like fine deletion.
Proposal: * Import the following NBTK classes: - Nbtk.Widget, Nbtk.Style, Nbtk.Stylable, Nbtk.Bin, Nbtk.Viewport - Nbtk.BoxLayout, Nbtk.BoxLayoutChild, Nbtk.Scrollable Further options: * Import Nbtk.ScrollView, Nbtk.Clipboard, Nbtk.Entry, and do the stylesheet work to make them look reasonable. Actually at this point we might as well import the whole thing * Rename Nbtk to something else (Tk?), move the shell widgets (ShellStack) etc. into Tk
Created attachment 142883 [details] [review] Import nbtk core Import the core NbtkWidget/NbtkBin and their dependencies.
Created attachment 142884 [details] [review] Import NbtkScrollView and dependencies
Created attachment 142885 [details] [review] [Nbtk] Add (transfer none) to some _get_default functions
Created attachment 142886 [details] [review] [Nbtk] NbtkViewport should respect parent bin's x-fill and y-fill properties Ensure NbtkViewport's allocate function respects x-fill and y-fill.
Created attachment 142887 [details] [review] [Nbtk] Remove hardcoded '28' from NbtkScrollView
Created attachment 142888 [details] [review] [ShellGlobal] Add a "datadir" property Will be used to load stylesheets from main.js.
Created attachment 142889 [details] [review] Load gnome-shell.css at startup
Created attachment 142890 [details] [review] Add css for scrolling Port bits of Nbtk's default.css into gnome-shell.css, and use some hand-rolled .pngs with colors from http://live.gnome.org/GnomeShell/DesignerPlayground/ExpandedViewMockups
Created attachment 142891 [details] [review] [LookingGlass] Port to nbtk
Created attachment 142892 [details] [review] [Nbtk] Import NbtkBoxLayout, NbtkBoxLayoutChild
Created attachment 142893 [details] [review] [Nbtk] Import NbtkEntry, NbtkLabel, NbtkClipboard For now this commit introduces an external dependency on clutter-imcontext.
Created attachment 142894 [details] [review] [Nbtk] Fix NbtkLabel and NbtkEntry's GObject properties
Created attachment 142895 [details] [review] [ShellGConf] Add shell_gconf_bind_object_property This allows having a GObject property slaved to a GConf key.
Created attachment 142896 [details] [review] [Nbtk] Add follow_style_font properties to NbtkEntry, NbtkLabel I want to be able to make both entries and labels which track the value of a GConf key, while at the same time inheriting style colors. Another approach would be to make yet more new entry/text classes, but...
Created attachment 142897 [details] [review] [Nbtk] Add border-width, border-color, corner-radius to NbtkWidget Implement this by taking BigRectangle and copying it to NbtkRectangle. When border-width is nonzero, we'll use the rectangle for borders.
Created attachment 142898 [details] [review] Port lookingGlass.js to Nbtk The main goal here is to move the style bits into CSS as a proof of concept.
Created attachment 142899 [details] [review] [LookingGlass] Port to nbtk The main goal here is to move the style bits into CSS as a proof of concept.
Tracking bug for upstreamable modifications here: http://bugzilla.moblin.org/show_bug.cgi?id=6053 Let me note non-upstreamable changes, of which I just remember one now: * Changed NbtkStyle.get_default() to not load the NBTK default.css (could potentially instead replace default.css with gnome-shell.css I guess)
Review of attachment 142885 [details] [review]: is this comprehensive, or just a few you noticed?
Review of attachment 142886 [details] [review]: ::: src/nbtk/nbtk-viewport.c @@ +287,3 @@ + natural_box.x2 = available_width; + else + natural_box.x2 = natural_width; I don't think you can keep calling it "natural box" if it isn't always the natural size
Review of attachment 142887 [details] [review]: Looks like some hacked in thing - the property is supposed to default to 24 as defined. Removing looks good
Review of attachment 142888 [details] [review]: Looks good
Review of attachment 142889 [details] [review]: Looks good
Review of attachment 142890 [details] [review]: I'm not going to say it looks good - using ID's in this fashion is offensive :-), but it looks like a reasonable cut-and-paste from nbtk. I think you should include a header comment in this file that that parts of it are adapted from nbtk, and has the NBTK copyright/license.
Review of attachment 142896 [details] [review]: Don't think this makes sense - the underlying label/entry is really an internal detail. The way it should work is: entry.style = "font: " + font_name + ";" This assumes that Pango font names are CSS font names, but that's close enough to true to be not worth worrying about deviations. libccss has inline styling support; it's not hooked up in libccss but it shouldn't be hard to add. (get_style vfunc of ccss_node_class_t) A little more problematical is parsing of the font shortcut for 'font-family/font-size/font-weight/font-style'. The facilities in libccss for doing this (used for border) are exposed so it could be added externally. But it's a little complex. (There's some parsing code for this in HippoCanvasStyle that could be adapted to handle the low-level part of going from libcroco CRTerm) As far as I can tell, there's also a problem in that libccss has no provision at all for properties like font and color that are supposed to be inherited by default, so the inline style would have to be bound to all widgets and not to just a parent widget. (All the properties that are "built in" to libccss like background and border do not inherit by default.) Not a blocking issue for this, but something important to resolve.
Review of attachment 142894 [details] [review]: Can't really review this without a commit message
Review of attachment 142895 [details] [review]: See my comment on attachment 142896 [details] [review] as to why I don't think this is the right way to do what you want to do. A convenience function to set the 'style' property of a NtkWidget based on a font name GCOnf key might make sense, but I think that makes more sense at the JS level.
Review of attachment 142892 [details] [review]: Looks fine. We need to figure out a way of handling clutter_container_child_set() - box.add_actor(child); box.get_child_meta(child).set({ expand: true; }); is ugly. What I suggested to Colin on IRC is to try monkey-patching NtkBox with: Nbtk.Box.prototype.add = function(child, props) { this.add_actor(child); if (props) this.get_child_meta(child).set(props); } [ add() might be a bit controversial, since clutter_container_add() exists as a varargs add-many that we don't bind. add_with_properties() would be uncontroversial, but cumbersome. ]
Review of attachment 142893 [details] [review]: clutter-imcontext needs to be added to the jhbuild, otherwise should be fine
Review of attachment 142897 [details] [review]: Mostly looks good to me: - Do we actually want to create a cut-and-paste copy of BigRectangle within our source tree, or should that wait until we are removing BigRectangle? - I'd like to see use of standard property names for border instead of non-standard names. See below. ::: src/nbtk/nbtk-widget.c @@ +1036,3 @@ nbtk_stylable_iface_install_property (iface, NBTK_TYPE_WIDGET, pspec); + pspec = g_param_spec_uint ("border-width", libccss has special parsing support for all standard CSS border properties, and you should piggy-back off of that rather than adding non-standard names. (probably along the lines of NbtkPadding in nbtk_style_fetch_ccss_property(). In the CSS model the border color and width are separate for all four sides and there are separate radiuses for all 4 corners. There are also different line styles. Supporting all of that is definitely not necessary. I'd be OK in fact if everything was just based off the top side and the top-right corner to start with. (I think separate border widths on the different sizes would be useful - that was supported in BigBox. but that can be done on demand.) See http://www.w3.org/TR/2002/WD-css3-border-20021107/ for border-radius, see that or the CSS21 spec for the rest of the border handling.
Review of attachment 142883 [details] [review]: libccss needs to be added to gnome-build-setup.sh and you need to check that it is in Ubuntu-9.4 if it's not in Ubuntu-9.4 we probably need to have it in the jhbuild. Otherwise looks good. ::: src/Makefile.am @@ -180,3 +183,3 @@ # (not the fake library, since we've already done the rewriting) Shell-0.1.typelib: libgnome-shell.la Shell-0.1.gir Big-1.0.gir - $(AM_V_GEN) LD_LIBRARY_PATH=.$${LD_LIBRARY_PATH:+:$$LD_LIBRARY_PATH} \ + $(AM_V_GEN) \ I'd rather this wasn't snuck in as part of this patch @@ -187,3 +190,3 @@ CLEANFILES += Shell-0.1.typelib -Big-1.0.gir: $(mutter) $(G_IR_SCANNER) libgnome-shell.la libbig-1.0.la $(srcdir)/big-enum-types.h Makefile +Big-1.0.gir: $(mutter) $(G_IR_SCANNER) libgnome-shell.la libbig-1.0.la Makefile Nor this @@ -205,2 +208,2 @@ Big-1.0.typelib: libbig-1.0.la Big-1.0.gir - $(AM_V_GEN) LD_LIBRARY_PATH=.$${LD_LIBRARY_PATH:+:$$LD_LIBRARY_PATH} \ + $(AM_V_GEN) $(G_IR_COMPILER) Big-1.0.gir -o $@ Nor this
Review of attachment 142884 [details] [review]: Assuming OK
Review of attachment 142899 [details] [review]: Geneally looks like a good way to go. Once some of the stuff commented in the various patches is fixed I think we can go ahead and land this. For NbtkBoxLayout vs. BigBox - I don't want to have a random mix of the two in our code. I think we should get a little experience of NbtkBoxLayout and see if it works well for what we need, and if so, then we should do a global "remove big box" pass at some point. ::: data/gnome-shell.css @@ +45,3 @@ +/* LookingGlass */ + +*#LookingGlassDialog *#LookingGlassDialog is the same as #LookGlassDialog - no reason to have the * there @@ +57,3 @@ +} + +*#LookingGlassDialog > *#Toolbar I really don't like using "id's" for non-unique - even if nbtk is already doing it. In our code #A #B Should always the be same as #B. So, here, use widget.style_class = "toolbar" and '#LookingGlass > .toolbar' @@ +74,3 @@ +} + +NbtkBoxLayout#LookingGlass > NbtkBoxLayout#EvalBox This is probably '#LookingGlass .eval-box' or maybe '#LookingGlassEvalBox' ::: js/ui/lookingGlass.js @@ +35,3 @@ +function _bindFontName(actor) { + Shell.GConf.get_default().bind_object_property(actor, 'font_name', '/desktop/gnome/interface/monospace_font_name'); +} See comments on other patches @@ +76,3 @@ })); + labelBox.add_actor(label); + labelBox.get_child_meta(label).expand = true; Commented on other patches
(In reply to comment #22) > Review of attachment 142885 [details] [review]: > > is this comprehensive, or just a few you noticed? I think it's comprehensive for the classes we imported, yeah. For _get_default I mean, not necessarily all annotations.
Review of attachment 142886 [details] [review]: Looking at this further, I think it's just wrong - xfill and yfill should affect the "viewport" and not the child widget. What would be desired here is something equivalent to gtk_scrolled_window_set_policy() - to say "I want the viewport to just be requested and allocated as big as the child in this direction" (Did notice a bug in the code portion that I filed as http://bugzilla.moblin.org/show_bug.cgi?id=6158)
Comment on attachment 142883 [details] [review] Import nbtk core Pushed to nbtk-introduction branch
Comment on attachment 142884 [details] [review] Import NbtkScrollView and dependencies Pushed to nbtk-introduction branch
Comment on attachment 142887 [details] [review] [Nbtk] Remove hardcoded '28' from NbtkScrollView Pushed to nbtk-introduction branch
Comment on attachment 142888 [details] [review] [ShellGlobal] Add a "datadir" property Pushed to nbtk-introduction branch
Comment on attachment 142889 [details] [review] Load gnome-shell.css at startup Pushed to nbtk-introduction branch
Comment on attachment 142890 [details] [review] Add css for scrolling Pushed to nbtk-introduction branch with a copyright header added to the CSS file
Comment on attachment 142892 [details] [review] [Nbtk] Import NbtkBoxLayout, NbtkBoxLayoutChild Pushed to nbtk-introduction branch
Comment on attachment 142893 [details] [review] [Nbtk] Import NbtkEntry, NbtkLabel, NbtkClipboard Pushed to nbtk-introduction branch with an addition of clutter-imcontext to the jhbuild moduleset. (You won't pick up the introduction unless you add something like moduleset = 'file:///home/<USERNAME>/gnome-shell/source/gnome-shell/tools/build/gnome-shell.modules' to your jhbuildrc-custom of course)
Created attachment 143308 [details] [review] Add GObject Introspection annotations Add GObject Introspection to methods where needed, in particular adding (transfer none) to return values that don't transfer ownership. clutter_texture_cache_get_actor() and clutter_texture_cache_get_texture() are annotated as (transfer none) since they return a newly created *floating* texture.
Comment on attachment 143308 [details] [review] Add GObject Introspection annotations Attachment 143308 [details] pushed as f1f11f1 - Add GObject Introspection annotations
Review of attachment 142894 [details] [review]: Quick further note on this Properties should be named according to the C getter's G_CONST_RETURN gchar *nbtk_label_get_text (NbtkLabel *label); ClutterActor * nbtk_label_get_clutter_text (NbtkLabel *label); G_CONST_RETURN gchar *nbtk_entry_get_text (NbtkEntry *entry); ClutterActor* nbtk_entry_get_clutter_text (NbtkEntry *entry); Well, except for the last one where the C getter needs to be renamed to get_clutter_entry()
Created attachment 143726 [details] [review] Add clutter-text properties to NbtkEntry and NbtkLabel Add clutter-text properties to allow getting access to the underlying ClutterText actor. This corresponds to the get_clutter_text() methods. The PROP_LABEL and PROP_ENTRY enum values are renamed to PROP_TEXT to match the names of the properties that they correspond to, and the properties of NbtkEntry are reordered into alphabetical order. Based on a patch from Colin Walters https://bugzilla.gnome.org/show_bug.cgi?id=591245 http://bugzilla.moblin.org/show_bug.cgi?id=6313
Created attachment 143744 [details] [review] Port LookingGlass console to NBTK * Style aspects like colors and fonts are moved into gnome-shell.css. * Scrolling is adding using NbtkScrollView. Based on a patch from Colin Walters
Created attachment 143745 [details] [review] lookingGlass: Get font from GConf Instead of using "Monospace", pick the users configured monospace font name up from GConf. (This is a nice touch, but is more done here to demonstrate that we can do it rather than for any great utility.)
Comment on attachment 142895 [details] [review] [ShellGConf] Add shell_gconf_bind_object_property Not useful, at least at the moment - patch 52 is a different way of doing the same thing, and for that the hook up to GConf needs to be in Javascript. This might be useful later, but hopefully we'll have some better way to bind GConf or we'll have moved to GSettings later.
Comment on attachment 142896 [details] [review] [Nbtk] Add follow_style_font properties to NbtkEntry, NbtkLabel Obsoleted by inline style
Created attachment 144331 [details] [review] Add clutter-text properties to StEntry and StLabel Add clutter-text properties to allow getting access to the underlying ClutterText actor. This corresponds to the get_clutter_text() methods. The PROP_LABEL and PROP_ENTRY enum values are renamed to PROP_TEXT to match the names of the properties that they correspond to, and the properties of StEntry are reordered into alphabetical order. Based on a patch from Colin Walters https://bugzilla.gnome.org/show_bug.cgi?id=591245 http://bugzilla.moblin.org/show_bug.cgi?id=6313
Created attachment 144332 [details] [review] Port LookingGlass console to ST widgets * Style aspects like colors and fonts are moved into gnome-shell.css. * Scrolling is adding using StScrollView. Based on a patch from Colin Walters
Created attachment 144333 [details] [review] lookingGlass: Get font from GConf Instead of using "Monospace", pick the users configured monospace font name up from GConf. (This is a nice touch, but is more done here to demonstrate that we can do it rather than for any great utility.)
Review of attachment 144331 [details] [review]: How about "renderer" instead of "clutter-text"?
Review of attachment 144332 [details] [review]: LGTM, if we decide to change clutter_text to something else we'll have to update this patch.
Review of attachment 144333 [details] [review]: LGTM
(In reply to comment #58) > Review of attachment 144331 [details] [review]: > > How about "renderer" instead of "clutter-text"? clutter-text matches the C getters: ClutterActor* st_entry_get_clutter_text (StEntry *entry); ClutterActor * st_label_get_clutter_text (StLabel *label); I could change them, but I think that would probably count as a gratuitous difference from Mx.
Attachment 144331 [details] pushed as b77b205 - Add clutter-text properties to StEntry and StLabel Attachment 144332 [details] pushed as d430449 - Port LookingGlass console to ST widgets Attachment 144333 [details] pushed as 1c7c53d - lookingGlass: Get font from GConf