[PATCH 2/2] tree_selected_foreach: ingore trips in the selection

[Dirk Hohndel]

On Sep 27, 2012, at 4:15 PM, Lubomir I. Ivanov wrote:

> On 28 September 2012 01:57, Linus Torvalds
> <torvalds at linux-foundation.org> wrote:
>> On Thu, Sep 27, 2012 at 3:45 PM, Lubomir I. Ivanov <neolit123 at gmail.com> wrote:
>>> From: "Lubomir I. Ivanov" <neolit123 at gmail.com>
>>> 
>>> Ignore trips (DIVE_INDEX == -1) when iterating trought the selection,
>>> so that we only call delete_single_dive() for dives.
>>> 
>>> Signed-off-by: Lubomir I. Ivanov <neolit123 at gmail.com>
>>> ---
>>> divelist.c | 10 +++++++---
>>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/divelist.c b/divelist.c
>>> index 7fa6bac..d1d4038 100644
>>> --- a/divelist.c
>>> +++ b/divelist.c
>>> @@ -1899,11 +1899,15 @@ struct tree_selected_st {
>>> static void tree_selected_foreach(GtkTreeModel *model, GtkTreePath *path,
>>>                                   GtkTreeIter *iter, gpointer userdata)
>>> {
>>> +       int idx;
>>>        struct tree_selected_st *st = (struct tree_selected_st *)userdata;
>>> 
>>> -       st->total++;
>>> -       st->list = (GtkTreeIter *)realloc(st->list, sizeof(GtkTreeIter) * st->total);
>>> -       memcpy(&st->list[st->total - 1], iter, sizeof(GtkTreeIter));
>>> +       gtk_tree_model_get(MODEL(dive_list), iter, DIVE_INDEX, &idx, -1);
>>> +       if (idx >= 0) {
>>> +               st->total++;
>>> +               st->list = (GtkTreeIter *)realloc(st->list, sizeof(GtkTreeIter) * st->total);
>>> +               memcpy(&st->list[st->total - 1], iter, sizeof(GtkTreeIter));
>>> +       }
>> 
>> Ugh. This is disgusting.
>> 
> 
> yeah i know,
> some of that is already in master. of course we can revert it if
> better solutions are viable…

I would really prefer a different solution. Convenient as the tree model is for
some things, here it seems to make much more sense to track things in the dives.
We will recreate the model, anyway.
All we need to do is make sure that the time stamps are adjusted.
Why can't we leave the tree model untouched, use it to update the time stamps
and to get rid of the dive_trip entries in the dive_trip_list. And then discard the
whole model and recreate it.
The more I think about it, that seems the way to go.
Does this make sense to you?

>> Why not just add a flag to the dive saying "delete me"? Do it like this:
>> 
>>    diff --git a/dive.h b/dive.h
>>    index 4d736b119b18..c04820e9611e 100644
>>    --- a/dive.h
>>    +++ b/dive.h
>>    @@ -252,7 +252,7 @@ struct dive {
>>        int number;
>>        tripflag_t tripflag;
>>        dive_trip_t *divetrip;
>>    -   int selected;
>>    +   unsigned int selected:1,deleted:1;
>>        timestamp_t when;
>>        char *location;
>>        char *notes;
>> 
>> and it won't use any extra space, and it's much easier for everybody to follow.
>> 
>> Then you can clean up the selected dives afterwards, without the
>> interaction with gtk iterators.
>> 
> 
> not sure it would work though? the two problems i've encountered were:
> 1) we should gtk_tree_selection_selected_foreach() instead of other
> methods, but should not modify the tree from it.

Correct. Don't modify it. We're throwing it away afterwards, anyway.

> 2) update_trip_timestamp() is the fastest way to modify the trip date
> based on remaining dives if needed, but it actually requires an
> updated tree.

It really doesn't. You can mark the dives as deleted and simply walk the
children of the trip in the tree to find the first and last child that are not deleted.
Still O(n) algorithm.

> the combination of the above led me to allocated the array.
> 
> there are possibly much better solutions...i.e. like you mention
> above, the list is repopulated anyway, so why not use dive, trip
> struct specific flags.
> divelist.c is getting quite entangled, i think.

It is. Tell me about it.

If this is getting too wild for you let me know and I'll take a stab at it.

>> And re-allocating the array on each addition is evil anyway. Ugh, what
>> horrible O(n**2) behavior, although some allocators may avoid some of
>> it thanks to extra slack.
>> 
> 
> i proposed allocating in chunks of say 128 GtkTreeIter values if that
> is better sound,
> however the root of the method is essentially a gtk api "workaround".
> outdated gtk versions, does not even provide a simple method to
> retrieve the selection length. :\

I would like to abandon that direction and go towards what I suggested above.

/D