Created attachment 143727 [details] [review]
Import stylesheet code from hippo-canvas

Import:

  HippoCanvasTheme      => ShellTheme
  HippoCanvasThemeImage => ShellThemeImage
  HippoCanvasStyle      => ShellThemeNode

ShellThemeContext is a new class managing the theme for a stage and
global properties like resolution.

test-theme.c is a newly written test program to do verification of the
style matching and property handling rules.

Various changes are made in the import:

 - Comprehensive reindentation
 - guint32 pixels replaced with ClutterColor
 - General pseudo-class support added
 - Old-fashioned (non-bordered) background image support added, though
   with no support for repeat, etc.
 - Bug fixes for problems revealed by test program

Created attachment 143728 [details] [review]
Port our imported parts of Nbtk to ShellTheme

ShellTheme replaces both NbtkStyle and ccss_stylesheet_t.

The interface NbtkStylable is replaced by usage of ShellThemeNode.
A concrete node class allows some significant optimizations of property
inheritance that would have been much more difficult to achieve with
the highly abstract pair of NbtkStylable and ccss_node_t.

Some operations that were previously on NbtkStylable (like the
::style-changed signal) are directly on NtkWidget.

Custom properties are no longer registered as param-specs; instead you
call directly into shell theme node to look up a length or color:

shell_theme_node_get_length (theme_node, "border-spacing", FALSE, &spacing);

The dependency on libccss is dropped, while preserving all existing
functionality and adding proper parsing and inheritance of font properties
and proper inheritance for the 'color' property.

Some more javascript tests for CSS functionality are added; workarounds for
a CSS bug where *.some-class was needed instead of .some-class are removed.

Created attachment 143729 [details] [review]
Fix problems with 4-sided padding: specifiers

The test for identifying such a specifier was wrong, and the last
value was assigned to the wrong sides.

Created attachment 143730 [details] [review]
Rename ShellThemeImage to ShellBorderImage

The current CSS3 border-image is close to a superset of what we were
doing for -hippo-background-image. Woot! rename ShellThemeImage to
ShellBorderImage and change parsing to look for:

 border-image: <url> <number>...

Rather than

 -shell-background-image: <url> <length>...

percentanges for the border sizes are not currently supported, neither
are the keywords for handling of the middle part. We always do 'stretch'
for now.

Created attachment 144316 [details] [review]
Import stylesheet code from hippo-canvas

Import:

  HippoCanvasTheme      => StTheme
  HippoCanvasThemeImage => StThemeImage
  HippoCanvasStyle      => StThemeNode

StThemeContext is a new class managing the theme for a stage and
global properties like resolution.

test-theme.c is a newly written test program to do verification of the
style matching and property handling rules.

Various changes are made in the import:

 - Comprehensive reindentation
 - guint32 pixels replaced with ClutterColor
 - General pseudo-class support added
 - Old-fashioned (non-bordered) background image support added, though
   with no support for repeat, etc.
 - Bug fixes for problems revealed by test program

Created attachment 144317 [details] [review]
Port our imported parts of Mx to ShellTheme

ShellTheme replaces both StStyle and ccss_stylesheet_t.

The interface StStylable is replaced by usage of ShellThemeNode.
A concrete node class allows some significant optimizations of property
inheritance that would have been much more difficult to achieve with
the highly abstract pair of StStylable and ccss_node_t.

Some operations that were previously on StStylable (like the
::style-changed signal) are directly on NtkWidget.

Custom properties are no longer registered as param-specs; instead you
call directly into shell theme node to look up a length or color:

shell_theme_node_get_length (theme_node, "border-spacing", FALSE, &spacing);

The dependency on libccss is dropped, while preserving all existing
functionality and adding proper parsing and inheritance of font properties
and proper inheritance for the 'color' property.

Some more javascript tests for CSS functionality are added; workarounds for
a CSS bug where *.some-class was needed instead of .some-class are removed.

Created attachment 144318 [details] [review]
Fix problems with 4-sided padding: specifiers

The test for identifying such a specifier was wrong, and the last
value was assigned to the wrong sides.

Created attachment 144320 [details] [review]
Rename StThemeImage to StBorderImage

The current CSS3 border-image is close to a superset of what we were
doing for -hippo-background-image. Woot! rename StThemeImage to
StBorderImage and change parsing to look for:

 border-image: <url> <number>...

Rather than

 -st-background-image: <url> <length>...

percentanges for the border sizes are not currently supported, neither
are the keywords for handling of the middle part. We always do 'stretch'
for now.

Comment on attachment 144316 [details] [review]
Import stylesheet code from hippo-canvas

As noted on IRC a gtk-doc section comment would be good.

>
>+static void
>+on_stage_destroy (ClutterStage *stage)
>+{
>+  StThemeContext *context = st_theme_context_get_for_stage (stage);
>+
>+  g_object_set_data (G_OBJECT (stage), "st-theme-context", NULL);

Won't the data go away when the stage is destroyed anyways?

>+/**
>+ * st_theme_context_get_for_stage:

This looks convenient, but it does mean that it's wrong/useless
to do new St.ThemeContext()?  Would it make sense to have
st_theme_context_bind (StThemeContext *context, ClutterStage *stage)
which this function could then just be:

{
  if (have_data) return context;
  context = new Context();
  st_theme_context_bind (context, stage);
  set_data ();
  return context;
}

If we don't want to allow this, then we should remove st_theme_context_new
from the public headers.

>+double
>+st_theme_context_get_resolution (StThemeContext *context)
>+{
>+  g_return_val_if_fail (ST_IS_THEME_CONTEXT (context), 96.);

Thsi could use a #define, same for the font.

Comment on attachment 144316 [details] [review]
Import stylesheet code from hippo-canvas


>+void
>+st_theme_context_set_resolution (StThemeContext *context,
>+                                 double          resolution)
>+{
>+  g_return_if_fail (ST_IS_THEME_CONTEXT (context));
>+
>+  context->resolution = resolution;
>+}
>
>+void
>+st_theme_context_set_font (StThemeContext             *context,
>+                           const PangoFontDescription *font)
>+{
>+  g_return_if_fail (ST_IS_THEME_CONTEXT (context));
>+
>+  if (context->font == font)
>+    return;
>+
>+  pango_font_description_free (context->font);
>+  context->font = pango_font_description_copy (font);
>+}

Is there a reason these don't emit CHANGED?

Comment on attachment 144316 [details] [review]
Import stylesheet code from hippo-canvas

>+#if 0
>+enum {
>+  LAST_SIGNAL
>+};
>+
>+static int signals[LAST_SIGNAL];
>+#endif

>+static void
>+st_theme_node_dispose (GObject *object)
>+{
>+  /* StThemeNode *node = ST_THEME_NODE (object); */
>+
>+  G_OBJECT_CLASS (st_theme_node_parent_class)->dispose (object);
>+}

cruft? (Likewise in st-theme.c)

>+static int
>+color_component_from_double (double component)
>+{
>+  /* http://people.redhat.com/otaylor/pixel-converting.html */
>+  if (component >= 1.0)
>+    return 255;
>+  else
>+    return (int)(component * 256);
>+}

the link doesn't really seem that relevant... this function is simpler than anything discussed on that page.

>+          if (num->type == NUM_PERCENTAGE)
>+            {
>+              value = num->val / 100;
>+            }

as mentioned on IRC, there's a lot of this (braces on 1-line if)

>+    case NUM_AUTO:
>+      g_warning ("'auto' not supported for lengths");
>+      return VALUE_NOT_FOUND;

Long-term, it seems like g_warnings are the wrong way to report parse errors.

>+st_theme_node_get_border_width (StThemeNode *node,
>+                                   StSide       side)

indentation

>+              if (result == VALUE_FOUND)
>+                {
>+                }
>+              else if (result == VALUE_INHERIT)

I don't really like the empty ifs, but if they're going to be kept around, there's also inconsistency between using "{}" or ";" for the body. (The ";"s come later on, eg in st_theme_node_get_font.)

>+          /* Can concetenate to bare words, but not two quoted strings */

two typos

>+++ b/src/st/st-theme-node.h

>+/* An element_type of G_TYPE_NONE means this style was created for the stage
>+ * actor and matches a selector element name of 'stage'
>+ */

hrmph. all these comments should move to the .c file. That's where I'm used to going to find documentation.

>diff --git a/src/st/st-theme.c b/src/st/st-theme.c

>+ *   coelesce properties with the same name.
coAlesce

>+ *  - Reformatted to match the gnome-st coding style
gnome-shell (search-and-replace-o?)

>+ *  - Some code simplication
simplification


>+static void st_theme_init (StTheme * theme)
>+{

st-theme.c has sporadic code style issues. (here, a missing newline, and a space after the "*"). Also, mixed tabs and spaces for indentation.

>+  /**
>+   * StTheme:application-stylesheet

needs a trailing ":" if it's supposed to be a real gtk-doc comment

>+/* This is a workaround for:
>+ *  - Python encodes '.' as '+' in type names

so is it relevant to us?

Comment on attachment 144317 [details] [review]
Port our imported parts of Mx to ShellTheme

i skimmed this and didn't notice anything wrong

(In reply to comment #5)
> Created an attachment (id=144316) [details]
> Import stylesheet code from hippo-canvas
> 
> Import:
> 
>   HippoCanvasTheme      => StTheme
>   HippoCanvasThemeImage => StThemeImage
>   HippoCanvasStyle      => StThemeNode

The meat of this is clearly StThemeNode; I can't say I found anything obviously wrong here in terms of errors, and danw got all the style.  I had some concern about how often we were looping over properties and strcmp'ing (while we do only expect nodes to have 4-5, this is multiplied by actor nesting, correc

(In reply to comment #6)

> shell_theme_node_get_length (theme_node, "border-spacing", FALSE, &spacing);

Might be possible to have as a "protected" method:

_shell_widget_bind (widget, "foreground-color, "color"); 

for the simpler cases?

I didn't see anything obviously wrong otherwise.

(In reply to comment #9)
> (From update of attachment 144316 [details] [review])
> As noted on IRC a gtk-doc section comment would be good.
> 
> >
> >+static void
> >+on_stage_destroy (ClutterStage *stage)
> >+{
> >+  StThemeContext *context = st_theme_context_get_for_stage (stage);
> >+
> >+  g_object_set_data (G_OBJECT (stage), "st-theme-context", NULL);
> 
> Won't the data go away when the stage is destroyed anyways?

qdata is only removed when a GObject is finalized, not when it it is disposed. Using set_data_full() and letting the theme context get unreffed at stage finalize would have been another option, but I generally like getting rid of as much as possible deterministically at dispose time.

> >+/**
> >+ * st_theme_context_get_for_stage:
> 
> This looks convenient, but it does mean that it's wrong/useless
> to do new St.ThemeContext()? 

It's of limited utility. There's nothing in StThemeContext that assumes that the theme context is actually associated with a stage (or even used to theme Clutter widgets). See test-theme.c for a usage. Added a doc comment to this effect.

> Would it make sense to have
> st_theme_context_bind (StThemeContext *context, ClutterStage *stage)

Can't really thik of when I'd want that.

> which this function could then just be:
>
> {
>   if (have_data) return context;
>   context = new Context();
>   st_theme_context_bind (context, stage);
>   set_data ();
>   return context;
> }
> 
> If we don't want to allow this, then we should remove st_theme_context_new
> from the public headers.

I think it's OK, with the doc comment. StThemeContext is a bit esoteric internals anyways, you only need to use it once when setting up the stylesheets for your app.

> >+double
> >+st_theme_context_get_resolution (StThemeContext *context)
> >+{
> >+  g_return_val_if_fail (ST_IS_THEME_CONTEXT (context), 96.);
> 
> Thsi could use a #define, same for the font.

Added.

(In reply to comment #10)
> (From update of attachment 144316 [details] [review])
> 
> >+void
> >+st_theme_context_set_resolution (StThemeContext *context,

> >+void
> >+st_theme_context_set_font (StThemeContext             *context,
> >+                           const PangoFontDescription *font)
> 
> Is there a reason these don't emit CHANGED?

No reason. They should.

Created attachment 144514 [details] [review]
StThemeContext fixes (Squash with 'Import stylesheet code from hippo-canvas')

- Emit changed when the default font or stylesheet changes.
- When emitting changed, drop the old root node
- Add function docs and a SECTION for the class
- Add #defines for default values

(In reply to comment #11)
> (From update of attachment 144316 [details] [review])
> >+#if 0
> >+enum {
> >+  LAST_SIGNAL
> >+};
> >+
> >+static int signals[LAST_SIGNAL];
> >+#endif
> 
> >+static void
> >+st_theme_node_dispose (GObject *object)
> >+{
> >+  /* StThemeNode *node = ST_THEME_NODE (object); */
> >+
> >+  G_OBJECT_CLASS (st_theme_node_parent_class)->dispose (object);
> >+}
> 
> cruft? (Likewise in st-theme.c)

Was our style for hippo-canvas and Mugshot - to always have the basic GObject machinery there and just comment out the unneeded parts. Removed.

> >+static int
> >+color_component_from_double (double component)
> >+{
> >+  /* http://people.redhat.com/otaylor/pixel-converting.html */
> >+  if (component >= 1.0)
> >+    return 255;
> >+  else
> >+    return (int)(component * 256);
> >+}
> 
> the link doesn't really seem that relevant... this function is simpler than
> anything discussed on that page.

Well, it's not intuitive, and that page does contain rational for doing it that way instead say *255. Extended the comment to say:

  /* We want to spread the range 0-1 equally over 0..255, but
   * 1.0 should map to 255 not 256, so we need to special-case it.
   * See http://people.redhat.com/otaylor/pixel-converting.html
   * for (very) detailed discussion of related issues. */

So the reader doesn't need to go to the link.

> >+          if (num->type == NUM_PERCENTAGE)
> >+            {
> >+              value = num->val / 100;
> >+            }
> 
> as mentioned on IRC, there's a lot of this (braces on 1-line if)

Fixed most of them.

> >+    case NUM_AUTO:
> >+      g_warning ("'auto' not supported for lengths");
> >+      return VALUE_NOT_FOUND;
> 
> Long-term, it seems like g_warnings are the wrong way to report parse errors.

Well, probably want some sort of CSS debugging console. The CSS specification is very clear on error handling - you are just supposed to ignore the entire property. This allows things like:

 border-radius: 10px;
 -moz-border-radius: 10px;

But we don't care about that. What I'm doing no mostly is just warning for stuff we don't support and silently ignore stuff that is invalid. Whihc isn't that useful. Filed bug 597019 to make warnings more consistent.

> >+st_theme_node_get_border_width (StThemeNode *node,
> >+                                   StSide       side)
> 
> indentation

Fixed.

> >+              if (result == VALUE_FOUND)
> >+                {
> >+                }
> >+              else if (result == VALUE_INHERIT)
> 
> I don't really like the empty ifs, but if they're going to be kept around,
> there's also inconsistency between using "{}" or ";" for the body. (The ";"s
> come later on, eg in st_theme_node_get_font.)

There's a reaosn {} if I'm using {} in the block, ; if I'm not. Sometimes ifs empty ifs are the clearest way to show structure, but yes, they can also look like someone forgot to finish what they were doing. I:

 - Added comments here:

              if (result == VALUE_FOUND)
                {
                  /* color stored in node->background_color */
                }

 - Coded around it in st_theme_node_get_font (used continue; instead)

> >+          /* Can concetenate to bare words, but not two quoted strings */
> 
> two typos

Fixed.

> >+++ b/src/st/st-theme-node.h
> 
> >+/* An element_type of G_TYPE_NONE means this style was created for the stage
> >+ * actor and matches a selector element name of 'stage'
> >+ */
> 
> hrmph. all these comments should move to the .c file. That's where I'm used to
> going to find documentation.

Wrote doc-comments for st_theme_node_new() and st_theme_node_get_length/color/double() to move the most "doc commentlike" part of these comments into the C file. (and add a bunch more infomration.)

> >diff --git a/src/st/st-theme.c b/src/st/st-theme.c
> 
> >+ *   coelesce properties with the same name.
> coAlesce
> >+ *  - Reformatted to match the gnome-st coding style
> gnome-shell (search-and-replace-o?)
> >+ *  - Some code simplication
> simplification

Fixed.
 
> >+static void st_theme_init (StTheme * theme)
> >+{
> 
> st-theme.c has sporadic code style issues. (here, a missing newline, and a
> space after the "*"). Also, mixed tabs and spaces for indentation.

It was run through indent to deal with much bigger issues, then manually fixed up back to our coding style. There are probably bigger issues with the code than the coding style. Fixed the ones you've noted and untabified it.

> >+  /**
> >+   * StTheme:application-stylesheet
> 
> needs a trailing ":" if it's supposed to be a real gtk-doc comment

Fixed.
 
> >+/* This is a workaround for:
> >+ *  - Python encodes '.' as '+' in type names
> 
> so is it relevant to us?

Probably not, though we'll have to see what happens when we have inheritance in JS. I've removed the hack, switched to g_type_is_a() and removed the comment about inefficiency.

Also added SECTION: headers for StTheme and StThemeNode.

(In reply to comment #13)
> (In reply to comment #5)
> > Created an attachment (id=144316) [details] [details]
> > Import stylesheet code from hippo-canvas
> > 
> > Import:
> > 
> >   HippoCanvasTheme      => StTheme
> >   HippoCanvasThemeImage => StThemeImage
> >   HippoCanvasStyle      => StThemeNode
> 
> The meat of this is clearly StThemeNode; I can't say I found anything obviously
> wrong here in terms of errors, and danw got all the style.  I had some concern
> about how often we were looping over properties and strcmp'ing (while we do
> only expect nodes to have 4-5, this is multiplied by actor nesting, correc

I have some define concerns about performance, though less than with the NbtkStylable code. Probably I'm more concerned about matching styles than finding properties. But I also have a bunch of ideas about optimization and there's good open-source prior art as well in the browsers.

If the loops become a problem, we can merge all of the border/padding/font loops into one loop and do the typical:

 switch (property_name[0])
  {
    case 'b':
     if (strcmp(property_name, "border") == )
       ...
  }

Inheritance isn't a problem for the standard properties, it doesn't magnify - because we inherit from the *cached* properties. So once we've compute the foreground color for some parent node once, we don't have to loop over its properties for every child. (And note that only foregroud color and font are inherited. Most properties aren't.)

(In reply to comment #14)
> (In reply to comment #6)
> 
> > shell_theme_node_get_length (theme_node, "border-spacing", FALSE, &spacing);
> 
> Might be possible to have as a "protected" method:
> 
> _shell_widget_bind (widget, "foreground-color, "color"); 
> 
> for the simpler cases?

Note that the code (for StLabel, say) is binding the foreground-color property to the color property of the child actor. There's an explicit policy here [see e.g. the later patch removing StBoxLayout:spacing when adding style property] of *not* mirroring style properties as properties.

Some things that could make add-hoc properties simpler:

 - Always queue resize when  the style changes.
 - Make st_theme_node_get_length() return integer pixels rather than a double
 - Add caching so st_theme_node_get_length() is efficient enough to just call when you need it, rather than caching in the widget's private structure. (it may be efficient enough now, but it was easier to leave the priv->spacing, etc, fields there rather than removing them.)

But I think that can evolve in the future.

Attachment 144316 [details] pushed as 276d9a9 - Import stylesheet code from hippo-canvas
Attachment 144317 [details] pushed as a9fd350 - Port our imported parts of Mx to ShellTheme
Attachment 144318 [details] pushed as 1fd2557 - Fix problems with 4-sided padding: specifiers
Attachment 144320 [details] pushed as 4d55ccf - Rename StThemeImage to StBorderImage