cedric pushed a commit to branch master.
http://git.enlightenment.org/core/efl.git/commit/?id=fd82c2521ebb9a324db8fdebd2c9a62b76ee6dc9
commit fd82c2521ebb9a324db8fdebd2c9a62b76ee6dc9
Author: Jean-Philippe Andre <jp.an...@samsung.com>
Date: Tue Oct 24 18:09:50 2017 +0900
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 <zm...@osg.samsung.com>
Signed-off-by: Cedric BAIL <ced...@osg.samsung.com>
---
src/lib/elementary/elm_genlist.c | 224 +++++++++++++++++++++++++++++++-
src/lib/elementary/elm_genlist.eo | 1 +
src/lib/elementary/elm_widget_genlist.h | 3 +
3 files changed, 227 insertions(+), 1 deletion(-)
diff --git a/src/lib/elementary/elm_genlist.c b/src/lib/elementary/elm_genlist.c
index 5caf6ae780..7e978c8b2a 100644
--- a/src/lib/elementary/elm_genlist.c
+++ b/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)
{
diff --git a/src/lib/elementary/elm_genlist.eo
b/src/lib/elementary/elm_genlist.eo
index 9f0949b7b5..8100b26077 100644
--- a/src/lib/elementary/elm_genlist.eo
+++ b/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;
diff --git a/src/lib/elementary/elm_widget_genlist.h
b/src/lib/elementary/elm_widget_genlist.h
index 7f22947416..fd4b68ac44 100644
--- a/src/lib/elementary/elm_widget_genlist.h
+++ b/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;
--
