Created attachment 353267 [details] [review]
st-entry: Add support for a "hint actor"

This allows a full ClutterActor to be used as hint in the entry, instead
of a simple string.

Created attachment 353268 [details] [review]
st-entry: Add support for the text-shadow CSS property

It's supported everywhere else, we should also support it here.

Created attachment 353269 [details] [review]
st-entry: Add primary-icon and secondary-icon read-write properties

Instead of simply providing a setter, we now provide real properties.

Review of attachment 353268 [details] [review]:

This needs better documentation - "Add support for the text-shadow CSS property" sounds like text-shadow support for entry text, but the patch appears to add some support under some specific (but unexplained) conditions only. Either make the patch implement the advertised behavior, or explain the limitations in the commit message and a comment.

Also a test case would be neat.

Review of attachment 353269 [details] [review]:

OK

Review of attachment 353267 [details] [review]:

This appears to break the existing hint-text property, at least in some cases (the overview search box). As that's even a simple case - hint-actor isn't used at all - I wonder if the best way to avoid odd interaction issues between both properties is to make the existing hint-text a shorthand:

 st_entry_set_hint_text (StEntry *entry, const char *text)
   {
     st_entry_set_hint_actor (entry, st_label_new (text));
   }

 st_entry_get_hint_text (StEntry *entry)
   {
     if (priv->hint_actor && ST_IS_LABEL (priv->hint_actor))
       return st_label_get_text (ST_LABEL (priv->hint_actor));
     return NULL;
   }

In any case, adding a test case to tests/interactive looks like a good idea as well.

Comment on attachment 353269 [details] [review]
st-entry: Add primary-icon and secondary-icon read-write properties

Attachment 353269 [details] pushed as 7f7d187 - st-entry: Add primary-icon and secondary-icon read-write properties

Created attachment 353280 [details] [review]
st: initialize static variables to NULL

Created attachment 353281 [details] [review]
st-entry: don't assume a cursor func has been set

That won't be the case when called from tests.

Created attachment 353282 [details] [review]
st-im-text: don't require st_im_text_set_event_window() to be called

Tests like tests/interactive/entry.js don't call it; make sure we still
display an entry in that case.

Created attachment 353283 [details] [review]
tests/entry: remove unused import

This is actually harmful, since it will drag a depepdency chain that
we're not able to satisfy from the tests.

Created attachment 353284 [details] [review]
st-entry: Add support for the text-shadow CSS property

It's supported everywhere else, we should also support it here.

Created attachment 353285 [details] [review]
tests/entry: add text-shadow

Also make the text larger and add an example string, since event
delivery does not seem to work properly under tests.

Created attachment 353286 [details] [review]
st-entry: don't possibly allocate actors a negative size

Clutter will complain about this, so protect the code against such edge
case.

Created attachment 353287 [details] [review]
st-entry: Add support for a "hint actor"

This allows a full ClutterActor to be used as hint in the entry, instead
of a simple string.

The string case has been now re-implemented on top of the hint actor.

Created attachment 353288 [details] [review]
tests/entry: add interactive tests for entry hints

Thanks for the reviews, Florian. I updated the patches and included testcases too (I had to fix a few things to get the entry testcase to run).

Review of attachment 353280 [details] [review]:

OK

Review of attachment 353281 [details] [review]:

OK

Review of attachment 353282 [details] [review]:

::: src/st/st-im-text.c
@@ +237,3 @@
+    {
+      g_object_unref (priv->window);
+      priv->window = NULL;

Or:
g_object_clear (&priv->window);

Review of attachment 353283 [details] [review]:

depepdency, otherwise LGTM

Review of attachment 353284 [details] [review]:

Overall this looks good to me, but shadow updates still don't quite work (but should be easy to fix)

::: src/st/st-entry.c
@@ +1065,3 @@
+    {
+      cogl_handle_unref (priv->text_shadow_material);
+      priv->text_shadow_material = COGL_INVALID_HANDLE;

This doesn't really work - that code is only called when the text was changed programmatically, not in the more common case where the user actually types in some text. I don't think we get around a ::text-changed handler ...

Review of attachment 353285 [details] [review]:

OK (the event delivery regression really is painful, we need to look into that eventually)

Review of attachment 353286 [details] [review]:

Is the issue that we use both the icons' min- and nat width in the size request, but always allocate them at their natural width? I don't mind the patch, I just wonder whether using the icons' nat width for the minimum size as well would fix the issue without starting to overlap elements ...

Review of attachment 353287 [details] [review]:

::: src/st/st-entry.c
@@ +278,3 @@
 static void
+st_entry_set_hint_visible (StEntry *self,
+                           gboolean visible)

As far as I can see, this could be

st_entry_update_hint_visibility (StEntry *self)

In the end, the visible parameter always boils down to

  strcmp (clutter_text_get_text (priv->entry, "") == 0 && !HAS_FOCUS (priv->entry),

so we could just as well compute it in the function itself. And maybe add "priv->hint_actor != NULL" to the conditions, so we don't set the indeterminate style when no hint has been set?

@@ +284,3 @@
+  if (visible)
+    {
+      priv->hint_visible = TRUE;

This was needed to differentiate between "priv->entry shows hint" and "priv->entry shows entry text". Now that priv->entry is never used for the hint, we don't need this anymore. In fact, I think the entire function can be reduced to:

gboolean visible = strcmp (st_entry_get_text (self), "") == 0 &&
                   !HAS_FOCUS (priv->entry);

if (priv->hint_actor)
  g_object_set (priv->hint_actor, "visible", visible, NULL);

if (visible)
  st_widget_add_style_pseudo_class (...);
else
  st_widget_remove_style_pseudo_class (...);

@@ +611,2 @@
   /* add a hint if the entry is empty */
+  if (g_strcmp0 (clutter_text_get_text (text), "") == 0)

Neither argument can be NULL, so strcmp

@@ +1143,2 @@
   else
+    st_entry_set_hint_visible (entry, FALSE);

Alternatively:

gboolean hint_visible = g_strcmp0 (text, "") == 0 && !HAS_FOCUS (priv->entry);
st_entry_set_hint_visible (entry, show_hint);

@@ +1451,3 @@
+        st_entry_set_hint_visible (entry, TRUE);
+      else
+        st_entry_set_hint_visible (entry, FALSE);

If we decide on not setting the :indeterminate style when no hint has been set, that bit should be moved out of the if block

::: src/st/st-entry.h
@@ +67,3 @@
 
+void            st_entry_set_hint_actor    (StEntry      *entry,
+                                            ClutterActor *hint_actor);

Maybe add a getter as well for the sake of completeness?

Review of attachment 353288 [details] [review]:

LGTM

(In reply to Florian Müllner from comment #26)
> Review of attachment 353288 [details] [review] [review]:
> 
> LGTM

(except that I don't see the neat swap-hints-every-second case, so maybe the problem here isn't even delivery, but the main loop?)

Attachment 353280 [details] pushed as d5cac65 - st: initialize static variables to NULL
Attachment 353281 [details] pushed as a256a35 - st-entry: don't assume a cursor func has been set
Attachment 353282 [details] pushed as 9e0e7a4 - st-im-text: don't require st_im_text_set_event_window() to be called
Attachment 353283 [details] pushed as 4c72244 - tests/entry: remove unused import

Created attachment 353770 [details] [review]
st-entry: Add support for the text-shadow CSS property

It's supported everywhere else, we should also support it here.

Created attachment 353771 [details] [review]
tests/entry: add text-shadow

Also make the text larger and add an example string, since event
delivery does not seem to work properly under tests.

Created attachment 353772 [details] [review]
st-entry: don't possibly allocate actors a negative size

Clutter will complain about this, so protect the code against such edge
case.

Created attachment 353773 [details] [review]
st-entry: Add support for a "hint actor"

This allows a full ClutterActor to be used as hint in the entry, instead
of a simple string.

The string case has been now re-implemented on top of the hint actor.

Created attachment 353774 [details] [review]
tests/entry: add interactive tests for entry hints

(In reply to Florian Müllner from comment #24)
> Review of attachment 353286 [details] [review] [review]:
> 
> Is the issue that we use both the icons' min- and nat width in the size
> request, but always allocate them at their natural width? I don't mind the
> patch, I just wonder whether using the icons' nat width for the minimum size
> as well would fix the issue without starting to overlap elements ...

That does not seem to be the issue... We do report the natural size of the icon in both the minimum and natural size of the entry as far as I can see. However under some circumstances (I believe this happened when booting without a monitor attached, but it's been a long time), it can happen that the entry is underallocated.

(In reply to Florian Müllner from comment #27)
> (In reply to Florian Müllner from comment #26)
> > Review of attachment 353288 [details] [review] [review] [review]:
> > 
> > LGTM
> 
> (except that I don't see the neat swap-hints-every-second case, so maybe the
> problem here isn't even delivery, but the main loop?)

Weird; I do see the swap here, so I don't think there's an issue with the mainloop though.

Thanks, Florian. I addressed the review comments and applied your suggestions to simplify further the hint actor patch.

Review of attachment 353770 [details] [review]:

LGTM

Review of attachment 353771 [details] [review]:

Reattached acn patch

Review of attachment 353772 [details] [review]:

OK, let's go with this then

Review of attachment 353773 [details] [review]:

Nice

Review of attachment 353774 [details] [review]:

Reattached acn patch

Thanks for all the review Florian!

Attachment 353770 [details] pushed as 8783654 - st-entry: Add support for the text-shadow CSS property
Attachment 353771 [details] pushed as 708f65e - tests/entry: add text-shadow
Attachment 353772 [details] pushed as 4e07d0b - st-entry: don't possibly allocate actors a negative size
Attachment 353773 [details] pushed as 6ed7034 - st-entry: Add support for a "hint actor"
Attachment 353774 [details] pushed as 47b109d - tests/entry: add interactive tests for entry hints