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. -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ------------------------------------------------------------------------------ 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
