Created attachment 121589 [details] [review]
Patch allows GtkLabel::use-underline to work with GtkLabel::attributes

As I learned with another patch, GSEAL() doesnt let
us randomly add members to public structs without
breaking abi, my misunderstanding, this patch needs
to be modified to add the new members as private
data (will do later tonight or tomorrow... just noting).

The general idea looks ok. A couple of comments:

- As you already mentioned, any new members need to go to the private struct. That being said, labels are _the_ most common widget, so I've tried very hard not to grow labels any further. Thus, I'd really like to avoid the extra two pointers if possible. 

- gtk_label_compose_effective_attrs looks like it could be done more effectively using mergesort, since you are basically merging two sorted lists. Maybe this should live in pango ?

- The documentation updates are missing.

Created attachment 121650 [details] [review]
Updated patch.

Ok this one only touches gtklabel.c :)

ok so as it is we need access to the attributes that markup sets,
the attributes that are manually set, and pattern/mnemonics can
be generated on demand, so...

In my personal opinion people never need manually set 
attributes and a markup string - it didnt occur to me
to propose it since it constitutes more of a behavioral
change, I think it would be good to store markup strings
in label->attrs when use-markup is TRUE.

In which case, we could re-render the mnemonics/pattern
into label->effective_attrs always merging with label->attrs
when available, and no new pointers.

How would that approach sound ?

about mergesort, I didnt come across that looking at
the pango_attr_list api but I'll look into it (Im sure
I was looking for it ! heh)... I doubt this change 
constitutes a change in pango.

Forget about the mergesort, that was more of a sidenote.

For the other proposal I'll have to read up the docs on how the various things are documented to interact right now.

Looking at the way the code currently works, I don't think we need to store the markup_attrs and underline_attrs. They are only needed in gtk_label_recalculate to recreate the effective_attrs, but gtk_label_recalculate parses the markup anyway, so we can just have markup_attrs and underline_attrs as local variables in gtk_label_recalculate. I think.

Created attachment 121796 [details] [review]
Much better approach

You were absolutely right, with a little effort I was able
to highly reduce the added code and get it all done,
and markup is not in conflict with manual attributes or anything.

Here it is :)

Created attachment 121928 [details]
test case

Looks refreshingly simple, indeed.

Here is a test case that shows that it does indeed make attributes and use-underline work together. It does not however, make attributes and markup work together. Was it intended to ?

Hmmm it was not, but it obviously could - err - I am not planning
to offer the user the option to set both in glade, I leave it to
you to decide if they should or should not work together in gtk+,
I suppose, if one were to set both, the expected results would
be a merge ?

Yeah, I would expect a merge. But it is certainly less important to make attributes and markup work together than attributes and underline.

So, I'd be ok with the patch as is, except for the missing documentation updates.

The gtk_label_set_attributes need to be updated.

Created attachment 121930 [details] [review]
new patch

This one updates the docs and merges attributes with markup strings.

 NOTE:

Please use <note><para> ... </para></note> here, so we get proper formatting in the docs. 

Also, this behaviour change probably needs a mention in README.in. Can you add
a "Release notes for 2.16" section and add the same content there ? 

With those changes, it looks good to commit.

Great, done done and closing :)