Page MenuHomePhabricator

genlist item ordering has changed
Closed, ResolvedPublic

Description

at some point this year item ordering within groups has changed.

in efl <= 1.19, group items were ordered like this internally:

  • item
  • item
  • item
  • parent

in efl >= 1.20, group items are ordered like this internally:

  • parent
  • item
  • item
  • item

now if a user of the api iterates list items with next/prev and then tries to get the parent, the results will be totally different, breaking existing apps.

example: https://git.enlightenment.org/apps/empc.git/tree/src/bin/empc.c#n3118
this code now crashes because getting the parent of the first item (which is now the parent) returns null when it should have instead returned the parent.

despite this change making logical sense, it's a significant behavior change and must be reverted.

zmike created this task.Aug 23 2017, 7:44 PM
zmike added a comment.Aug 23 2017, 7:47 PM

to be clear, it's the behavior change which must be reverted, the internal code could potentially remain unchanged. if all the functions like item_first/last/next/prev_get() had workarounds to preserve old behavior then this would probably be fine?

jpeg added a comment.Aug 23 2017, 8:07 PM

Yeah this change broke behavior.
I absolutely hate it but it seems more and more clear to me that we need bug compatibility, and this starts with a new API to define the target version an app was written for.
Without this information I don't know how we could implement the required workaround (legacy behavior).

jpeg added a subscriber: raster.Aug 23 2017, 10:04 PM

@cedric @raster this is a policy question (bug compatibility)

well we have to keep compatibility... that is totally true
but was it REALLy ordered that way 1.19 and before? really?

jpeg added a comment.Aug 27 2017, 8:02 PM

Yes. It was complete madness.

zmike added a comment.Aug 28 2017, 3:47 AM
In T5938#96636, @jpeg wrote:

Yes. It was complete madness.

jpeg added a comment.Oct 24 2017, 7:52 PM

Flat lists should work mostly fine both before and after my patches.

I can not find any good solution. I have prepared a patch (in my local branch) that "fixes" first/last/prev/next to simulate the old behavior. But this is sure to bring more issues as the internal code itself relies on the API "first_get". Basically the entire tree structure of genlist has always been broken by design, and still is, due to the unhealthy mix of list trees and flat inlist.

The described behavior here was not documented. In fact the documentation seems to imply that "next" should not have returned a parent or a child, only siblings. It doesn't say NULL, though. But it definitely doesn't imply anything about the internal order. The documentation for "prev" is worse, and those for "first" and "last" don't mention anything about parenting. IOW this behavior is completely out of specs.

The only absolutely awful solution I have in mind is to actually have a separate class for genlist-legacy and another one for genlist 1.20+. The code for genlist-legacy would basically be a copy from EFL 1.19. This would obviously require some API target that the application specifies, like:

int main(...) {
   efl_api_set(1, 20);
   ...
}

I really don't care much about EMPC here. I worry about other applications. Honestly I am not sure how anyone could write any traversal code for genlist without knowing the internals very well and then going

oh shit this is totally broken and doesn't make sense at all

The only way I can think of to keep compatibility is to keep the same code as in 1.19 and not touch it ever. That would be perfect bug compatibility.

jpeg added a comment.Oct 24 2017, 7:57 PM

Note that my changes fixed:

  • long lasting odd/even issue
  • invalid order with sorted insert (in tree case)
  • crash with sorted insert (in tree case)
  • first item is the topmost one, last item is the bottom most, next item is right below, etc... (logical order - unrelated to tree vs. flat)
jpeg added a comment.Oct 24 2017, 10:34 PM

Actually I can probably keep smashing my head against the wall and implement "compatibility" code but:

  • This still requires an explicit "EFL API target" API (and if unknown then assume 1.8, all compatibility code is enabled - also print warnings).
  • The exact bugs^Wbehaviors will not be the same as in <=1.19.
jpeg added a comment.Oct 25 2017, 6:57 AM

zmike added a comment.Oct 26 2017, 7:20 AM

It would be trivial to fix empc to use the new and more sane ordering--it would have taken me less time than creating this ticket--but the point is that this is a behavior change.

The attached patch seems to restore old behavior from my testing.

jpeg added a comment.Oct 26 2017, 6:45 PM
In T5938#102990, @zmike wrote:

It would be trivial to fix empc to use the new and more sane ordering--it would have taken me less time than creating this ticket--but the point is that this is a behavior change.

Obviously :)

The attached patch seems to restore old behavior from my testing.

It just simulates the old behavior in a limited number of cases.
To be fair, assuming I didn't add a new potential crash, the simulated code is probably more robust than the original code (due to the high number of issues there were).

Do you think we should go with this patch or copy & paste the original version?
We need the "API target" API no matter what (efl_api_set(1, 19) or something like that).

The last alternative is to just give up and not implement any workaround, blame the apps, and pray that only empc was broken by my changes (in community or Tizen).

zmike added a comment.Oct 27 2017, 4:57 AM

I think anyone who is using genlist item iterating with groups is broken by that patch. The release has been out for several months now and nobody else has complained, which means that either nobody is using that or nobody has updated.

I'm fine with just removing genlist just to see if anyone notices, but I suppose the more responsible thing to do would be to use your patch since it seems more maintainable?

jpeg added a comment.Oct 31 2017, 7:35 PM
In T5938#103058, @zmike wrote:

I'm fine with just removing genlist just to see if anyone notices

Good idea, that will save us a lot of trouble ;)

I'm noticing now a break in my mediacenter most probably caused by the @jpeg patch applyed.

In my genlist with groups I'm no more able to iterate over all the items, when I call item_next on a group item I get the next group item instead of the next visual item.

My genlist is (visually) something like this:

item #1
GROUP #1
item #2
item #3
GROUP #2
item #4

if I call item_next on GROUP #1 I get GROUP #2 instead of item #2

This works as expected in stable 1.20, while it is broken in git EFL.

I did not understand if this is a new wanted behaviour or is just a bug (it seems a bug to me)

zmike added a comment.Apr 11 2018, 1:01 PM

Unfortunately, if you wrote anything based on the item ordering behavior from 1.19 or 1.20 then your application is broken and must be fixed.

The point is that the new behaviour of item_next_get() seems wrong to me.
Really we want item_next() on a group item to return the next group item instead of the normal next item?

The behavior that I would expect is that item_next(GROUP #1) would return item #4, assuming that item #4 belongs to GROUP #2. If it is not part of a group then the behavior you are seeing is correct.

maybe I wrote the graph wrong, as your last comment seems non-sensical to me, I try again:

item #1
GROUP #1
 - item #2 
 - item #3
GROUP #2
 - item #4

in this scenario item_next(GROUP #1) currently return GROUP #2, while I expect (as is returned in 1.20) to return Item #2
... for sure not item #4

maybe I wrote the graph wrong, as your last comment seems non-sensical to me, I try again:

item #1
GROUP #1
 - item #2 
 - item #3
GROUP #2
 - item #4

in this scenario item_next(GROUP #1) currently return GROUP #2, while I expect (as is returned in 1.20) to return Item #2
... for sure not item #4

You are getting the behavior I would expect. I would expect the ordering to be:
Item 1 -> Group 1 -> Group 2 ... with no item 2, 3 or 4 as they are subitems. Think of it as a directory tree. your genlist is listing /home/davemds:
/home/DaveMDS/Pictures
/home/DaveMDS/Documents

  • /home/DaveMDS/Documents/Personal
  • /home/DaveMDS/Documents/Professional

/home/DaveMDS/Videos

  • /home/DaveMDS/Videos/Movies

If I was iterating these items I would expect the order to be /home/DaveMDS/Pictures -> /home/DaveMDS/Documents -> /home/DaveMDS/Videos (because they are all on the same level) and then be done and not give sub items at all... If I was wanting to iterate the subitems too the way you want... functions exist for this. Ephoto does it... you would do something like
item... item_get() if (elm_genlist_item_expanded_get) { elm_genlist_item_subitems_get() }

In otherwords I don't think that item_)next/previous should delve into subitems at all... I think it should stay on the same level (as it does) and you can then delve into subitems with subitems_get

@stephenmhouston I agree with you in the case of a tree genlist, while I was talking just about groups.

btw, this is a behaviour break and is simply inacceptable in a released library, we are not even fixing a bugged-behavior, It's just a different one.

I see there is also the code in genlist to keep compatibility with older release, I cannot see why we don't keep this one.

ah you aren't using a tree.

I restored the old behaviour with commit rEFLf0a0da9f449b

I think this only apply to legacy genlists and I hope I have not broken something else...
let me know if my commit broke something for you. It fix epymc for me :)

I restored the old behaviour with commit rEFLf0a0da9f449b

I think this only apply to legacy genlists and I hope I have not broken something else...
let me know if my commit broke something for you. It fix epymc for me :)

This changes behavior again - not just preserving legacy - so not exactly a simple patch and not exactly a good idea to just yolo push change behavior - you should have posted the patch here first and let others review. That said -- As long as no other apps are depending on the ordering your are changing , and as long as nothing is broken, it will probably pass.

This changes behavior again - not just preserving legacy - so not exactly a simple patch and not exactly a good idea to just yolo push change behavior - you should have posted the patch here first and let others review. That said -- As long as no other apps are depending on the ordering your are changing , and as long as nothing is broken, it will probably pass.

my patch restore the behaviour that we currently have in stable 1.20 releases. I'm not getting your point...

zmike added a comment.Apr 13 2018, 2:58 PM

If make check continues to pass now then probably more tests should be added there to ensure compatibility.

@zmike ah, you right, the test is failing now, sorry I didn't know there was a test for this.

But it's the test that is wrong, as it is checking the new "broken" behavior, should I update the test to reflect the correct/legacy behavior?

hmm, there must be some difference on how epymc and empc use the genlist group feature. The test case @zmike wrote assume a behavior that is different from the one I was getting in efl 1.19 and 1.20... I'm a bit confused now...

The only idea in my mind atm is that the behavior of next/prev was different based on the insertion order,
In epymc I add items and group items in the order they appear, like:

add(GROUP#1)
add(item#1, parent=GROUP#1)
add(item#2, parent=GROUP#1)
add(GROUP#2)
add(item#3, parent=GROUP#2)
add(item#4, parent=GROUP#2)

@zmike: are you doing the add in a different order? maybe you first add all the groups and then all the items?

zmike added a comment.Apr 14 2018, 7:40 AM

@zmike ah, you right, the test is failing now, sorry I didn't know there was a test for this.

But it's the test that is wrong, as it is checking the new "broken" behavior, should I update the test to reflect the correct/legacy behavior?

The test is correct, do not change it. If the test no longer passes then you have broken behavior.

hmm, there must be some difference on how epymc and empc use the genlist group feature. The test case @zmike wrote assume a behavior that is different from the one I was getting in efl 1.19 and 1.20... I'm a bit confused now...

As I said, the behavior in 1.19 and 1.20 is broken. If you've written any application which depends on the behavior from this version then your application is broken. If you've pushed changes which have broken the tests which ensured compatibility (and fixed the broken ordering which was present in these versions), then those changes must be reverted.

The only idea in my mind atm is that the behavior of next/prev was different based on the insertion order,
In epymc I add items and group items in the order they appear, like:

add(GROUP#1)
add(item#1, parent=GROUP#1)
add(item#2, parent=GROUP#1)
add(GROUP#2)
add(item#3, parent=GROUP#2)
add(item#4, parent=GROUP#2)

@zmike: are you doing the add in a different order? maybe you first add all the groups and then all the items?

empc uses sorted insertion, but this is not relevant.

maybe I wrote the graph wrong, as your last comment seems non-sensical to me, I try again:

item #1
GROUP #1
 - item #2 
 - item #3
GROUP #2
 - item #4

in this scenario item_next(GROUP #1) currently return GROUP #2, while I expect (as is returned in 1.20) to return Item #2
... for sure not item #4

The point of this ticket is that legacy item ordering makes no sense and the behavior of item iteration functions is insane, hence the names of the functions in the patch from @jpeg. item #4 SHOULD DEFINITELY, 100% BE RETURNED if you call item_next(GROUP #1). That's why the test exists to verify this behavior. This is not a bug. If this is not what is returned from a genlist created with elm_genlist_add, then genlist must be fixed and the tests should be updated to have a specific case to test iterating from a group item.

If you want sane item ordering, do not use legacy API.

so what you are saying is that the test you wrote works on stable 1.20 ? I don't think so...

Ok, I run an extrapolated version of the test on current git (without my latest commit) and on 1.20.6
You can see the source I used here: F3025190

and this is the results:

Running on EFL: 1.20.99.58295
test 1: OK
test 2: OK
test 3: OK
test 4: OK
test 5: OK
test 6: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 8: OK
Running on EFL: 1.20.6
test 1: OK
test 2: FAILED
test 3: FAILED
test 4: FAILED
test 5: FAILED
test 6: FAILED
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 8: FAILED

As you can see the test do not pass on stable release.
Is this expected?
The test is not testing the correct behaviour (that is why my application no longer works with GIT version)

zmike added a comment.Apr 16 2018, 6:49 AM

This will be the last time that I can repeat myself; I no longer have even a shred of interest in this ticket or in what you decide to do after, since it seems that you are just willfully ignoring every reply I post:

THE BEHAVIOR FROM GENLIST ITEM ITERATION IN 1.20 AND 1.19 IS BROKEN AND ANY TESTS, APPS, OR OTHER CODE WRITTEN WHICH USES THIS BEHAVIOR IS ALSO BROKEN.

This is not a case of "let me try the tests in master on top of code from 1.20" or "let me provide compatibility with code from 1.20". This is a case of two major releases having shipped with code which provides behavior which is inconsistent with the previous 7+ years (and 18 stable releases).

It's completely irrelevant what you wrote based on these releases; code which functions correctly using those releases is fundamentally broken.

It's completely irrelevant whether tests from master pass when backported to those releases; the tests themselves are broken if they pass on those releases.

Behavior which has existed for this amount of time must be preserved regardless of how stupid or backwards it is.

zmike removed a subscriber: zmike.Apr 16 2018, 6:49 AM

@zmike ah, you right, the test is failing now, sorry I didn't know there was a test for this.

But it's the test that is wrong, as it is checking the new "broken" behavior, should I update the test to reflect the correct/legacy behavior?

Patch needs to be reverted.

raster added a comment.EditedApr 16 2018, 9:27 PM

@zmike definitely has a point. yes. 2 releases had broken behavior. but many more releases before that did not. sometimes some behavior is so outrageous is needs to change to be fixed. @jpeg's work does fix that. code relying on the broken behavior that has been fixed, even if it was in 2 releases is not accounting for code that seemingly was ok prior to that.

so i agree with @zmike here and @stephenmhouston.

DaveMDS reopened this task as Open.Apr 20 2018, 11:33 AM

I spent the last week studing this issue and setting up 9 virtual machines to test the behaviour in the last 9 stable releases of efl and elementary (a really long and annoying task!).

The results say that the first assumpion of this ticket (genlist order has always been insane) is just wrong!

We have had insane ordering only in EFL 1.18, all the releases after and before was sane. So I really think the @jpeg patch must be reverted, the @zmike genlist test must be adjusted to test for a sane ordering and a new legacy behaviour must be implemented (if needed).

I already written a new (sane) test and I run both tests in all the 9 releases + latest git, you can find the test I used here: F3030218
and this are the result of all the tests:

Running on EFL: 1.20.99.58357

INSANE TEST
test 1: OK
test 2: OK
test 3: OK
test 4: OK
test 5: OK
test 6: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 8: OK

SANE TEST
test 1: FAILED
test 2: FAILED
test 3: FAILED
test 4: OK
test 5: FAILED
test 6: FAILED
test 7: FAILED
test 8: FAILED
test 9: FAILED
Running on EFL: 1.20.7

INSANE TEST
test 1: OK
test 2: FAILED
test 3: FAILED
test 4: FAILED
test 5: FAILED
test 6: FAILED
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 8: FAILED

SANE TEST
test 1: OK
test 2: OK
test 3: OK
test 4: OK
test 5: OK
test 6: OK
test 7: OK
test 8: OK
test 9: OK
Running on EFL: 1.19.2

INSANE TEST
test 1: OK
test 2: FAILED
test 3: FAILED
test 4: FAILED
test 5: FAILED
test 6: FAILED
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 8: FAILED

SANE TEST
test 1: OK
test 2: OK
test 3: OK
test 4: OK
test 5: OK
test 6: OK
test 7: OK
test 8: OK
test 9: OK
Running on EFL: 1.18.5

INSANE TEST
test 1: OK
test 2: OK
test 3: OK
test 4: OK
test 5: OK
test 6: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 8: OK

SANE TEST
test 1: FAILED
test 2: FAILED
test 3: FAILED
test 4: OK
test 5: FAILED
test 6: FAILED
test 7: FAILED
test 8: FAILED
test 9: FAILED
Running on EFL: 1.17.1
ERR<1008>:eio lib/eio/eio_monitor.c:339 eio_monitor_stringshared_add() monitored path '/home/dave/.elementary/config/standard' not found.

INSANE TEST
test 1: OK
test 2: FAILED
test 3: FAILED
test 4: FAILED
test 5: FAILED
test 6: FAILED
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 8: FAILED

SANE TEST
test 1: OK
test 2: OK
test 3: OK
test 4: OK
test 5: OK
test 6: OK
test 7: OK
test 8: OK
test 9: OK
Running on EFL: 1.16.1

INSANE TEST
test 1: OK
test 2: FAILED
test 3: FAILED
test 4: FAILED
test 5: FAILED
test 6: FAILED
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 8: FAILED

SANE TEST
test 1: OK
test 2: OK
test 3: OK
test 4: OK
test 5: OK
test 6: OK
test 7: OK
test 8: OK
test 9: OK
Running on EFL: 1.15.3

INSANE TEST
test 1: OK
test 2: FAILED
test 3: FAILED
test 4: FAILED
test 5: FAILED
test 6: FAILED
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 8: FAILED

SANE TEST
test 1: OK
test 2: OK
test 3: OK
test 4: OK
test 5: OK
test 6: OK
test 7: OK
test 8: OK
test 9: OK
Running on EFL: 1.14.3

INSANE TEST
test 1: OK
test 2: FAILED
test 3: FAILED
test 4: FAILED
test 5: FAILED
test 6: FAILED
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 8: FAILED

SANE TEST
test 1: OK
test 2: OK
test 3: OK
test 4: OK
test 5: OK
test 6: OK
test 7: OK
test 8: OK
test 9: OK
Running on EFL: 1.13.3

INSANE TEST
test 1: OK
test 2: FAILED
test 3: FAILED
test 4: FAILED
test 5: FAILED
test 6: FAILED
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 8: FAILED

SANE TEST
test 1: OK
test 2: OK
test 3: OK
test 4: OK
test 5: OK
test 6: OK
test 7: OK
test 8: OK
test 9: OK
Running on EFL: 1.12.3

INSANE TEST
test 1: OK
test 2: FAILED
test 3: FAILED
test 4: FAILED
CRI<990>:elementary elm_genlist.c:6309 elm_genlist_item_parent_get() Elm_Widget_Item (Elm_Widget_Item *)it is NULL
test 5: FAILED
test 6: FAILED
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 7: OK
test 8: FAILED

SANE TEST
test 1: OK
test 2: OK
test 3: OK
test 4: OK
test 5: OK
test 6: OK
test 7: OK
test 8: OK
test 9: OK

@DaveMDS thank you very much for that. that is pretty good and useful data. so given that data i'd flip and say you're right here. your patch changed behavior back to 1.20/1.19 and 1.17 and before behavior right? all we need is for tests to pass with your changes in place then. right? last i knew there was work on that. so go back to f0a0da9f449b0878fe6a5ce2abc50b8b6589c50a and revert 01a95d1f40b5ecc3a6357dfe2451b319685603b6 right? i don't remember if tests past after f0a0da9f449b0878fe6a5ce2abc50b8b6589c50a was applied or not.

I think we just need to revert rEFLfd82c2521ebb and adjust the test case, I can do both as I already have a correct test to use.

but... I'm stalled at another confusion point: the doc for item_next() say:

Also the next item means item in the same tree level. If a item has subitems, and it have expand all subitems will be ignore, and will get the next item in the same level.

This piece of doc comes from rEFLc321e152a941, does this means that item_next() should never goes into child ? also if they are expanded?
In my experience this has never been the case, item_next has always returned the next VISUAL item, without keeping in consideration the parent/child relationship.

So (if I'm right with this) we need to:

  1. revert rEFLfd82c2521ebb
  2. adjust the test case
  3. fix the docs (reverting rEFLc321e152a941)

I can perform those 3 point but I would like to hear at least the @jpeg voice on this, as he have done all the previous work.

tests to see what has been the case over time?

I now tested the behaviour in all last 9 release, using the visual test elementary_test -to "genlist group tree" as per rEFL558f4c36ac7f

EFL 1.12 -> Flat
EFL 1.13 -> Flat
EFL 1.14 -> Flat
EFL 1.15 -> Flat
EFL 1.16 -> Flat
EFL 1.17 -> Flat
EFL 1.18 -> insane
EFL 1.19 -> Flat
EFL 1.20 -> Flat

Conslusion: documenttation is wrong, or really bad written. next always return the next VISUAL item regardless the tree structure

I will wait some more days to hear feedbacks and then will procede with the 3 points above if nothing new arise

then that sounds easy... fix the docs. :) but i think we have a general solution to this with data backing what should be done. i like that. it shouldn't lead to any arguments i think.

zmike added a comment.Apr 23 2018, 4:01 AM

Looks like I was wrong.

Thanks for looking into this despite opposition, let's bring back the longest standing ordering, update the docs, and also update the elm tests in src/tests to ensure that this never happens again.

zmike added a comment.Apr 23 2018, 4:08 AM

Also, there's no point in waiting on @jpeg here since he will not be replying.

And lets make sure we test a lot of other areas before moving ahead. Item ordering changed for a lot of reasons that were broke with genlist, not the least of which was how odd/even theme signals were being relayed due to broke ordering, so lets make sure everything is sane before pushing this behavior back.

Please post a patch for review here first so we can test before it is pushed.

@stephenmhouston there is no patch to post, you just need to revert rEFLfd82c2521ebb to test.