genlist: fix "insane" order [BUG COMPATIBILITY]

This patch implements bug compatibility.

genlist internally uses both a tree structure with Eina_List and a flat
Eina_Inlist to track its items. ALL of the items are in the inlist while
subitems appear in their parent's list. As a consequence both lists must
be kept in sync pretty tightly. Obviously this is not done at all and
has led to countless bugs, as soon as tree or groups are used:
 - Invalid order of items (visually)
 - Invalid order of items with sorted_insert
 - Glitches in the matrix
 - Crashes with sorted_insert
 - Odd/even styles not properly set
 - Promote/demote functions broken by design
 - Developers send to psychiatric hospitals
 - Etc...

Legacy genlist (1.19 and before) used an inlist order that basically
didn't make sense, as it didn't follow the logical order of elements (as
they appear visually). Unfortunately this has "worked" (really, that's a
huge stretch to use this word here) for a long time this way. As a
consequence, some applications (*cough* empc *cough*) have relied on
this order to implement "next album" or "previous album" where the
album title is a group node.

By changing the order of items in the inlist, this has broken the
assumptions made above, and ends up in cases that return NULL, leading
to SEGV. Sure, the app should have checked NULL, but that's not really
the point here. The behavior has been changed.

This patch implements "fixes" for the following functions:
 - elm_genlist_first_item_get(): Don't return a parent
 - elm_genlist_last_item_get(): Return a parent
 - elm_genlist_item_next_get(): return a parent upon reaching the last child
 - elm_genlist_item_prev_get(): return a child when a parent is passed

Important notes:
 - This does not cover 100% behavior compatibility here. The only way to
   have it would be to simply revert the entire genlist code to its
   original version and never touch it again, ever.
 - An explicit API is required for an application to specify which API
   level it targets, so that we can cherry-pick which bug compatibility
   features we want to enable. We are already doing this for EDC,
   unfortunately.

@fix

fix T5938

Signed-off-by: Mike Blumenkrantz <zmike@osg.samsung.com>
Signed-off-by: Cedric BAIL <cedric@osg.samsung.com>

src/lib/elementary/elm_genlist.c

@@ -5846,6 +5846,20 @@ _elm_genlist_efl_object_constructor(Eo *obj, Elm_Genlist_Data *sd)
    return obj;
 }
 
+EOLIAN static Eo *
+_elm_genlist_efl_object_finalize(Eo *obj, Elm_Genlist_Data *sd)
+{
+   obj = efl_finalize(efl_super(obj, MY_CLASS));
+
+   // FIXME:
+   // Genlist is a legacy-only widget which means it will always be marked as
+   // "legacy" here. The proper test here is "app targets EFL 1.19 or below".
+   if (elm_widget_is_legacy(obj))
+     sd->legacy_order_insane = EINA_TRUE;
+
+   return obj;
+}
+
 static void
 _internal_elm_genlist_clear(Evas_Object *obj)
 {
@@ -6813,10 +6827,208 @@ _elm_genlist_at_xy_item_get(const Eo *obj EINA_UNUSED, Elm_Genlist_Data *sd, Eva
    return NULL;
 }
 
+/* ======== Genlist legacy item ordering fix. Insanity begins here. ========
+ *
+ * Welcome to insanity land, where items' real order is not what matters.
+ *
+ * Sane order:
+ *
+ * parent1
+ *   parent2
+ *     leaf1
+ *     leaf2
+ *   leaf3
+ *   parent3
+ *     leaf4
+ *     leaf5
+ *
+ * Legacy "insane" order:
+ *
+ *     leaf1
+ *     leaf2
+ *   parent2
+ *   leaf3
+ *     leaf4
+ *     leaf5
+ *   parent3
+ * parent1
+ *
+ * Then filtering comes into play. Fun times.
+ * The implementation has undefined behavior. Don't worry. This is already
+ * insane anyway.
+ *
+ */
+
+static inline Elm_Gen_Item *
+_insane_item_unfiltered_child_find(Eina_Bool filter, Elm_Gen_Item *it_parent)
+{
+   Eo *eo_it = NULL;
+   Elm_Gen_Item *it, *it2;
+   Eina_List *li;
+
+   EINA_SAFETY_ON_NULL_RETURN_VAL(it_parent->item, NULL);
+   EINA_LIST_FOREACH(it_parent->item->items, li, eo_it)
+     {
+        it = efl_data_scope_get(eo_it, ELM_GENLIST_ITEM_CLASS);
+        if (!filter || _item_filtered_get(it))
+          {
+             it2 = _insane_item_unfiltered_child_find(filter, it);
+             if (it2) return it2;
+             else return it;
+          }
+     }
+
+   if (!eo_it) return NULL;
+   return it_parent;
+}
+
+static Elm_Object_Item *
+_elm_genlist_first_item_get_insane(Elm_Genlist_Data *sd)
+{
+   Elm_Gen_Item *it, *it2;
+
+   it = ELM_GEN_ITEM_FROM_INLIST(sd->items);
+   if (!it) return NULL;
+
+   // Find first unfiltered item (sane order)
+   while (it)
+     {
+        if (!sd->filter || _item_filtered_get(it)) break;
+        it = ELM_GEN_ITEM_NEXT(it);
+     }
+   if (!it) return NULL;
+
+   // Find a "leaf" item that isn't filtered...
+   it2 = _insane_item_unfiltered_child_find(sd->filter, it);
+   if (it2) it = it2;
+
+   return EO_OBJ(it);
+}
+
+static Elm_Object_Item *
+_elm_genlist_last_item_get_insane(Elm_Genlist_Data *sd)
+{
+   Elm_Gen_Item *it;
+
+   if (!sd->items) return NULL;
+
+   // Find last unfiltered item (sane order)
+   it = ELM_GEN_ITEM_FROM_INLIST(sd->items->last);
+   while (it)
+     {
+        if (!sd->filter || _item_filtered_get(it)) break;
+        it = ELM_GEN_ITEM_PREV(it);
+     }
+   if (!it) return NULL;
+
+   // Parents are not filtered out and are after the items, in insane order.
+   while (it->parent)
+     it = it->parent;
+
+   return EO_OBJ(it);
+}
+
+static Elm_Object_Item *
+_elm_genlist_next_item_get_insane(Elm_Genlist_Data *sd, Elm_Gen_Item *it)
+{
+   Elm_Gen_Item *it2;
+
+   if (sd->filter && !_item_filtered_get(it))
+     {
+        // If this item is filtered out, give up on smarts and just find
+        // the next filtered one.
+        for (it2 = ELM_GEN_ITEM_NEXT(it); it2; it2 = ELM_GEN_ITEM_NEXT(it2))
+          if (!sd->filter || _item_filtered_get(it2))
+            break;
+        return EO_OBJ(it2);
+     }
+
+   for (it2 = ELM_GEN_ITEM_NEXT(it); it2; it2 = ELM_GEN_ITEM_NEXT(it2))
+     {
+        // 0. If filtered out, go to next
+        if (sd->filter && !_item_filtered_get(it2))
+          continue;
+
+        // 1. Return next sibling in list, if any
+        if (it->parent == it2->parent)
+          return EO_OBJ(it2);
+
+        // 2. If next item is a child, skip until next sibling
+        if (it2->parent == it)
+          {
+             Eo *eo_it2 = NULL;
+             Eina_List *last;
+
+             EINA_SAFETY_ON_NULL_RETURN_VAL(it->item, NULL);
+             last = _list_last_recursive(it->item->items);
+             if (last) eo_it2 = last->data;
+             it2 = efl_data_scope_get(eo_it2, ELM_GENLIST_ITEM_CLASS);
+             continue;
+          }
+
+        // 3. If next item has another parent (or NULL parent), return it's parent
+        // Note: it's parent can't be filtered out, unless it was also filtered out.
+        if (it->parent && (it->parent != it2->parent))
+          return EO_OBJ(it->parent);
+     }
+   /* if item is already last item, return its parent if a parent exists */
+   if (it->parent)
+     return EO_OBJ(it->parent);
+   return EO_OBJ(it2);
+}
+
+static Elm_Object_Item *
+_elm_genlist_prev_item_get_insane(Elm_Genlist_Data *sd, Elm_Gen_Item *it)
+{
+   Elm_Gen_Item *it2, *parent;
+
+   if (sd->filter && !_item_filtered_get(it))
+     {
+        // If this item is filtered out, give up on smarts and just find
+        // the previous filtered one.
+        for (it2 = ELM_GEN_ITEM_PREV(it); it2; it2 = ELM_GEN_ITEM_PREV(it2))
+          if (!sd->filter || _item_filtered_get(it2))
+            break;
+        return EO_OBJ(it2);
+     }
+
+   parent = it->parent;
+   if (!parent)
+     {
+        Elm_Object_Item *eo_it2;
+        Eina_List *l;
+
+        // No parent: just return previous filtered item we can find.
+        EINA_LIST_REVERSE_FOREACH(it->item->items, l, eo_it2)
+          {
+             ELM_GENLIST_ITEM_DATA_GET(eo_it2, itt);
+             if (!sd->filter || _item_filtered_get(itt)) return eo_it2;
+          }
+        for (it2 = ELM_GEN_ITEM_PREV(it); it2; it2 = ELM_GEN_ITEM_PREV(it2))
+          if (!sd->filter || _item_filtered_get(it2))
+            break;
+        return EO_OBJ(it2);
+     }
+
+   it2 = ELM_GEN_ITEM_PREV(it);
+   if (it2 == parent)
+     return _elm_genlist_prev_item_get_insane(sd, it2);
+
+   return EO_OBJ(it2);
+}
+
+/* ======== Genlist legacy item ordering fix. Insanity ends here. ========== */
+
+
 EOLIAN static Elm_Object_Item*
 _elm_genlist_first_item_get(Eo *obj EINA_UNUSED, Elm_Genlist_Data *sd)
 {
-   Elm_Gen_Item *it = ELM_GEN_ITEM_FROM_INLIST(sd->items);
+   Elm_Gen_Item *it;
+
+   if (EINA_LIKELY(sd->legacy_order_insane))
+     return _elm_genlist_first_item_get_insane(sd);
+
+   it = ELM_GEN_ITEM_FROM_INLIST(sd->items);
 
    while (it && sd->filter && !_item_filtered_get(it))
      it = ELM_GEN_ITEM_NEXT(it);
@@ -6829,6 +7041,9 @@ _elm_genlist_last_item_get(Eo *obj EINA_UNUSED, Elm_Genlist_Data *sd)
 {
    Elm_Gen_Item *it;
 
+   if (EINA_LIKELY(sd->legacy_order_insane))
+     return _elm_genlist_last_item_get_insane(sd);
+
    if (!sd->items) return NULL;
    it = ELM_GEN_ITEM_FROM_INLIST(sd->items->last);
 
@@ -6843,6 +7058,9 @@ _elm_genlist_item_next_get(Eo *eo_it EINA_UNUSED, Elm_Gen_Item *it)
 {
    ELM_GENLIST_DATA_GET_FROM_ITEM(it, sd);
 
+   if (EINA_LIKELY(sd->legacy_order_insane))
+     return _elm_genlist_next_item_get_insane(sd, it);
+
    do it = ELM_GEN_ITEM_NEXT(it);
    while (it && sd->filter && !_item_filtered_get(it));
 
@@ -6854,6 +7072,9 @@ _elm_genlist_item_prev_get(Eo *eo_it EINA_UNUSED, Elm_Gen_Item *it)
 {
    ELM_GENLIST_DATA_GET_FROM_ITEM(it, sd);
 
+   if (EINA_LIKELY(sd->legacy_order_insane))
+     return _elm_genlist_prev_item_get_insane(sd, it);
+
    do it = ELM_GEN_ITEM_PREV(it);
    while (it && sd->filter && !_item_filtered_get(it));
 
@@ -7709,6 +7930,7 @@ _filter_item_internal(Elm_Gen_Item *it)
 }
 
 // Returns true if the item is not filtered out, but remains visible.
+// If a parent is filtered out its children are also filtered out.
 static Eina_Bool
 _item_filtered_get(Elm_Gen_Item *it)
 {

src/lib/elementary/elm_genlist.eo

@@ -529,6 +529,7 @@ class Elm.Genlist (Efl.Ui.Layout, Efl.Ui.Focus.Composition, Elm.Interface_Scroll
    implements {
       class.constructor;
       Efl.Object.constructor;
+      Efl.Object.finalize;
       Efl.Gfx.position { set; }
       Efl.Gfx.size { set; }
       Efl.Canvas.Group.group_member_add;

src/lib/elementary/elm_widget_genlist.h

@@ -205,6 +205,9 @@ struct _Elm_Genlist_Data
 
    Eina_Bool                             tree_effect_animator : 1;
    Eina_Bool                             pin_item_top : 1;
+
+   // If true, use insane legacy item order: item, item, parent
+   Eina_Bool                             legacy_order_insane : 1;
 };
 
 typedef struct _Item_Block Item_Block;