Created attachment 109040 [details] [review]
pango attributes parsing in GtkLabel

As per ancient bug 97061, editing labels in glade would be much
more powerful if attributes were handled separately from label
text, its also inconvenient for translators to have pango markup
in their strings.

This patch parses PangoAttributes onto GtkLabel in the
following form:
  <object class="GtkLabel" name="foo_label">
    <pango>
      <attribute name="weight" value="PANGO_WEIGHT_BOLD"/>
    </pango>
  </object>

The patch also includes tests to buildertest.c.

Note, just got a better grip on how pango attributes work

TODO: implement byte ranges:
  <attribute name="weight" value="PANGO_WEIGHT_BOLD" start="0" end="6"/>

would span the entire "weight" text.

Comment on attachment 109040 [details] [review]
pango attributes parsing in GtkLabel

Thanks, this looks pretty good!
Just a couple of minor comments:

>Index: gtk/gtklabel.c

[..]
>+pango_start_element (GMarkupParseContext *context,
[..]
>+	      g_set_error (error,
>+			   GTK_BUILDER_ERROR,
>+			   GTK_BUILDER_ERROR_INVALID_ATTRIBUTE,
>+			   "%s:%d:%d '%s' is not a valid attribute of <%s>",
>+			   "<input>",
>+			   line_number, char_number, names[i], "attribute");
[..]
>+	  g_set_error (error,
>+		       GTK_BUILDER_ERROR,
>+		       GTK_BUILDER_ERROR_MISSING_ATTRIBUTE,
>+		       "%s:%d:%d <%s> requires attribute \"%s\"",
>+		       "<input>",
>+		       line_number, char_number, "attribute",
>+		       name ? "value" : "name");

These two errors are not tested, can they be?

>+gtk_label_buildable_custom_tag_start (GtkBuildable     *buildable,
[..]
>+  if (strcmp (tagname, "pango") == 0)

I think "attributes" would make more sense here, don't you?
This is a GtkLabel specific tag which provides something which only makes sense in a label.

>+test_pango_attributes (void)
>+{
[..]
>+  g_assert (found.weight);
>+  g_assert (found.foreground);
>+  g_assert (found.underline);
>+  g_assert (found.size);
>+  g_assert (found.language);
>+  g_assert (found.font_desc);

Freeing attributes manually should not be required by the callsite, it should be enough to unref the label.

Created attachment 109063 [details] [review]
New patch with start and end properties implemented and fixed tests

Ok Im refitting this patch a little bit... about the tests, well
the call really doesnt ever need to call gtk_label_get_attributes()
in practice, its the only way I figured how to test that they were
actually applied to the label.

Im kindof indifferent about the name, changing it to <attributes> for
this patch, I thought, like <accessibility> tag, it would add context
that the <attribute> tags were in fact <pango><attribute/></pango>s ;-)

So by the way this updated patch also includes and tests start/end
properties, so that you can set properties to portions of your text.

I imagine we may want to replicate this code in textbuffer or such,
maybe at that point we can make attribute_from_text () privately 
shared.

(In reply to comment #4)
> Created an attachment (id=109063) [edit]
> New patch with start and end properties implemented and fixed tests
> 
> Ok Im refitting this patch a little bit... about the tests, well
> the call really doesnt ever need to call gtk_label_get_attributes()
> in practice, its the only way I figured how to test that they were
> actually applied to the label.

Oh, I didn't look carefully enough. Forgot that.

> I imagine we may want to replicate this code in textbuffer or such,
> maybe at that point we can make attribute_from_text () privately 
> shared.

Sounds good.

I guess this can go in, but Mattias would appreciate if you got in some documentation, now when we have all other tags properly documented.

and I'd like to see parser/GError tests, to make sure that the error paths actually work, I tried quickly to do the same in another custom tag implementation without luck.

this patch included:

  g_assert (error);

in buildertest.c to assure that builder would error out
on faulty xml... is that good enough for a test ?

So then I take it I can commit this and move on to document
the added custom tags. I'll open other bugs for parsing
attributes onto other widgets.

Hi Tristan!

Is is it only editable via UI file ATM?
I can't see any option in GtkLabel properties pane.

Thanks a lot :)

Yes I am going to do that somewhere abouts this week,
a dialog on the GtkLabel properties that allows you
to specify global attributes, or attributes that apply
to specific parts of the set label text.

Thanks Tristan, you are doing a great work! ;)