Created attachment 166581 [details] [review]
[StContainer] Add :first-child, :last-child pseudo classes

Add support for the CSS :first-child and :last-child properties to
StContainer, and thus to all containers in St and ShellGenericContainer.

The internal ordering of the container's children is used to determine
the child to which to attach the pseudo class, not the children's
positions. This means that containers where positions can differ from
the ordering (ShellGenericContainer / StGroup) may behave unexpectedly,
so some caution is required.

(In reply to comment #0)
> See patch. The pseudo classes are arguably only useful with support for
> non-uniform borders / border-radii

nah, there are cases where it's useful to have different padding on the ends. (I wanted this in the original message tray summary area.)

Review of attachment 166581 [details] [review]:

I think it's fine if it works funny for ShellGenericContainer.... if you are creating a ShellGenericContainer where the :first-child/:last_child pseudo-classes are meaningful, you can take care of keeping the child list in the appropriate order. (Or if it becomes an issue, we can shell_generic_container_set_auto_first_last_child() and you can manage the pseudo-classes yourself.) Patch looks basically fine to me, except for some details of the logic.

::: src/st/st-container.c
@@ +53,3 @@
+  StContainerPrivate *priv = container->priv;
+
+  first_item = g_list_first (priv->children);

I really don't like g_list_first personally. I don't think you can use GList without understanding a linked list, and once you know what a linked list is, g_list_first (list) is obviously just list. [I suspect that you were trying for symmetry with the use of g_list_last() - I think that's sort of a fake symmetry - getting the last item of a list is very different than getting the first item of a list]

@@ +55,3 @@
+  first_item = g_list_first (priv->children);
+  if (first_item == NULL)
+    return;

This logic isn't right - if first_item is null, you still might need to remove the first_child/last_child classes from the old children. (I'm assuming that we don't want to leave classes on children that have been removed from the container.)

@@ +66,3 @@
+        st_widget_add_style_pseudo_class (ST_WIDGET (first_child),
+                                          "first-child");
+      priv->first_child = first_child;

A reference should be added to priv->first_child here. Otherwise, when you do this after remove()/remove_all(), priv->first_child might be pointing to unallocated memory and removing the style from it will crash.

@@ +372,3 @@
+
+  if (priv->last_child)
+    priv->last_child = NULL;

This leaves the pseudo-classes on the old children.

Created attachment 166927 [details] [review]
[StContainer] Add :first-child, :last-child pseudo classes

Add support for the CSS :first-child and :last-child properties to
StContainer, and thus to all containers in St and ShellGenericContainer.

Comment on attachment 166927 [details] [review]
[StContainer] Add :first-child, :last-child pseudo classes

>+      if (priv->first_child)
>+        {
>+          if (ST_IS_WIDGET (priv->first_child))
>+            st_widget_remove_style_pseudo_class (ST_WIDGET (priv->first_child),
>+                                                 "first-child");
>+          g_object_unref (priv->first_child);
>+          priv->first_child = NULL;
>+        }

it seems ugly to have this twice (once for the no children case, once for the children-changed case). A simple "first_child = first_item ? first_item->data : NULL" would get rid of the duplicate cases...

>+  container->priv->first_child = NULL;
>+  container->priv->last_child = NULL;

you don't really need to do that. priv is initialized to 0s.

Created attachment 167199 [details] [review]
[StContainer] Add :first-child, :last-child pseudo classes

OK.

Attachment 167199 [details] pushed as 0905940 - [StContainer] Add :first-child, :last-child pseudo classes