On Sat, Nov 30, 2013 at 12:49 AM, Carsten Haitzler <ras...@rasterman.com> wrote: > On Fri, 29 Nov 2013 14:08:16 -0200 Gustavo Sverzut Barbieri > <barbi...@gmail.com> said: > >> On Thu, Nov 28, 2013 at 10:17 PM, Carsten Haitzler <ras...@rasterman.com> >> wrote: >> > On Thu, 28 Nov 2013 20:22:21 -0200 Gustavo Sverzut Barbieri >> > <barbi...@gmail.com> said: >> > >> >> Hi, >> >> >> >> I was debugging the crash with elm_genlist_clear() in 1.8 and I'm >> >> wondering the rationale of "rel". Currently it is badly broken given >> >> that you use anything other than append/prepend without parent. >> >> >> >> The current code keeps a list of items in the smart data (sd->items). >> >> Although these are ordered as they would be displayed, the code will >> >> replicate that information in the "rel" (it->items->rel), that means >> >> the item the current one is relative to. Then it keeps a boolean >> >> "before" flag and a reverse relative list (who is relative to it). >> > >> > i remember putting in rel ONLY for the purpose of the queue handling. ie >> > the >> > item is queued but since it is relative TO something, rel is filled in with >> > before true/false. after the item is no longer in the queue - rel should be >> > unused and ignored. we could happily set it to NULL if we wanted. >> >> Good to know, reseting these flags would make lots of sense, avoid >> bugs (as the one I fixed with _item_del_pre_hook, or at least make it >> harder to show as it would still happen if the item wasn't realized >> yet) and save memory (you'd not keep the list in memory). >> >> While at that, I'd say it's better to remove the "rel" and "before" >> from items and make it a separate structure that is allocated once the >> item is relative to something (you'd only have a single pointer wasted >> in the main structure for the item, not a pointer + bitflag, that if >> padding is wrong may spawn over one pointer in size!). Similarly we >> could have rel_revs to not be a list, but a structure with two >> pointers... if we fix this bug keeping the "rel", then we'll always >> have at most one before and another after, so 2 ptrs. >> >> >> >> Bad things happens if you insert or move before, after or sorted as >> >> the siblings are not updated. Take _item_move_before(it, before): >> >> >> >> sd->items = >> >> eina_inlist_remove(sd->items, EINA_INLIST_GET(it)); >> >> if (it->item->block) _item_block_del(it); >> >> sd->items = eina_inlist_prepend_relative >> >> (sd->items, EINA_INLIST_GET(it), EINA_INLIST_GET(before)); >> >> >> >> So far, so good. remove from old position and place it where it should >> >> be. Delete the block so it would be recreated when needed. >> >> >> >> >> >> if (it->item->rel) >> >> it->item->rel->item->rel_revs = >> >> eina_list_remove(it->item->rel->item->rel_revs, it); >> >> >> >> It was relative to another item, then remove the current item from the >> >> reverse relative list of that other item as it's not relative to it >> >> anymore. Good. >> >> >> >> it->item->rel = before; >> >> before->item->rel_revs = eina_list_append(before->item->rel_revs, it); >> >> it->item->before = EINA_TRUE; >> >> >> >> Now make the item relative to the given parameter "before", also >> >> appending itself to its reverse relative list. Flag as "before". Just >> >> that? >> >> >> >> Did you notice the problem? If not take the given list as input: >> >> - a (rel = , before=False, rel_revs=[c]) >> >> - c (rel = a, before=False, rel_revs=[d]) >> >> - d (rel = c, before=False, rel_revs=[b]) >> >> - b (rel = d, before=False, rel_revs=[]) >> >> >> >> Now move "b" before "c". The result will be: >> >> - a (rel = , before=False, rel_revs=[c]) >> >> - b (rel = c, before=True, rel_revs=[]) >> >> - c (rel = a, before=False, rel_revs=[d, b]) >> >> - d (rel = c, before=False, rel_revs=[]). >> >> >> >> Wait, "c" is still relative to "a" even if should be after "b"... To >> >> fix this is quite cumbersome as other elements would be relative given >> >> they can be before=True or False. Would need to walk the reverse >> >> relative's list of "c" and see if "b" got in the way of them, changing >> >> their relative members. >> >> >> >> Looking at the code it seems the user of that is _item_block_add(), >> >> does it need all of that complexity to do its job? I didn't review >> >> such function, but if we could remove all those pointer+boolean+list >> >> we would save memory AND complexity. >> > >> > but then you'd be forced to evaluate all queued items immediately. either >> > that or re-architect how the queue works. ie insert items immediately into >> > the actual list AND keep a queue entry, but this will have all sorts of >> > nasty knock-on effects when a user scrolls as they will encounter entire >> > block or sets of items not yet sized and identified. >> >> well, the current approach is also wrong, so we need to find a solution. >> >> Can't we insert in the sd->items list and somehow flag before/after >> item blocks as gone, then we just go and regenerate them? (that may >> not make sense, I never looked into the block handling of genlist) > > thats what i was mentioning. change the architecture to put the items in > directly, but then we need to know if a block is only partly processed or > fully, or if a block is unprocessed at all, and then our queue walking > possibly > need to just walk blocks, tracking the first block that has at least 1 item > un-processed, then process the block (quickly skipping already processed > items), etc. etc. - this would be slower to do it this way, but save having a > queue list at all. > > another way is to keep the queue, and just walk it as now, but also insert > items into the items list and blocks right away, but all logic that assumes a > queued item is never in a block or the items list needs review/updating. > > these aren't going to happen before 1.8 release :) it's a major change there.
Great, 1.8 was done. Now could someone fix this annoying bug? -- Gustavo Sverzut Barbieri -------------------------------------- Mobile: +55 (19) 9225-2202 Contact: http://www.gustavobarbieri.com.br/contact ------------------------------------------------------------------------------ Rapidly troubleshoot problems before they affect your business. Most IT organizations don't have a clear picture of how application performance affects their revenue. With AppDynamics, you get 100% visibility into your Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro! http://pubads.g.doubleclick.net/gampad/clk?id=84349351&iu=/4140/ostg.clktrk _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
