Page MenuHomePhabricator

Revert "elm_genlist: when appending items to the parent, prepend to the parent"
AbandonedPublic

Authored by zmike on Jul 12 2016, 12:09 AM.

Details

Summary

This reverts commit 43d82e567a2d655a089b6ca3f2d913e6ec52f1dc.

That commit breaks iternal items order: subitems are prepended to parent item,
but still drawn in correct position. Because of that prev/next items are
returned incorrectly.

Test Plan
  1. open elementary_test
  2. open Genlist Tree
  3. expand first item
  4. press Up button

(expected result: nothing happens, without reverting - last subitem selected)

  1. select first item again
  2. press Down button

(expected result: first subitem selected, without reverting - all subitems skipped)

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2292
Build 2357: arc lint + arc unit
an.kroitor updated this revision to Diff 9570.Jul 12 2016, 12:09 AM
an.kroitor retitled this revision from to Revert "elm_genlist: when appending items to the parent, prepend to the parent".
an.kroitor updated this object.
an.kroitor edited the test plan for this revision. (Show Details)
an.kroitor added reviewers: zmike, cedric, raster.
an.kroitor added subscribers: rimmed, NikaWhite, FurryMyad.
raster edited edge metadata.Jul 12 2016, 6:00 PM

i'm not so sure. bug a or bug b. that commit looks like another bug fix... i am unsure exactly which bug and how to produce it... zmike should know.

zmike edited edge metadata.Jul 13 2016, 6:40 AM

This is an interesting issue. The original commit that you've referenced fixes a significant bug with non-tree groups (eg. "Genlist Group" test) where items would be appended after their group item, breaking the group item (no content/text able to be displayed).

In the case of this type of group item, it is guaranteed to be wrong to have any of the parent item's subitems after the group item, so there must be a check here to prevent that. It seems, however, that tree hierarchies also use this same codepath, and they do not require special handling.

It seems to me that the choices are:

  • Revert this and then rework object layout for header-type groups to handle the case of the parent not being after all of its items
  • Add another check for the case I added to only apply it in non-tree cases

The second option seems far simpler here.

@zmike could you provide some steps how to break "Genlist Group" example? I've checked it with and without reverting that commit, but looks like example is unaffected.

zmike added a comment.Jul 13 2016, 7:29 AM

None of the current tests trigger that codepath. It occurs when _list_last_recursive() returns NULL.

But _list_last_recursive() returns NULL only if sub-items list is empty, right? I can't still figure out how to reproduce it.

zmike commandeered this revision.May 1 2018, 10:01 AM
zmike edited reviewers, added: an.kroitor; removed: zmike.

I think this is related to T5938 and so it probably is not relevant anymore.

zmike abandoned this revision.May 1 2018, 10:01 AM