[PATCH v3 6/7] comctl32/listbox: Implement LBS_NODATA for single-selection listboxes

[Gabriel Ivăncescu]

On Wed, Nov 21, 2018 at 4:51 PM Huw Davies <huw at codeweavers.com> wrote:
>
> > +static BOOL NODATA_is_sel(const LB_DESCR *descr, UINT index)
> > +{
> > +    return index == descr->selected_item;
> > +}
>
> The idea would be to put all of this logic into is_item_selected().
>

Sure. I was just trying to separate all of the NODATA internal stuff
into their own helpers.

> > +
> > +static BOOL NODATA_multisel_expand(LB_DESCR *descr, UINT num)
> > +{
> > +    LB_ITEMDATA *p = descr->items;
> > +    UINT sz = num * sizeof(*p);
> > +
> > +    if (!p || sz > HeapSize(GetProcessHeap(), 0, p))
> > +    {
> > +        sz += LB_ARRAY_GRANULARITY * sizeof(*p) - 1;
> > +        sz -= sz % (LB_ARRAY_GRANULARITY * sizeof(*p));
> > +        if (!p) p = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sz);
> > +        else  p = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, p, sz);
> > +        if (!p) return FALSE;
> > +        descr->items = p;
> > +    }
> > +    return TRUE;
> > +}
>
> You shouldn't be adding modata muiltsel stuff in this patch.
>

Yeah I mentioned it in the comments on the first iteration I sent. The
reason I did it this way was to avoid having to keep the old SetCount
lingering around.

Which means I'd have to either (1) use a goto to minimize patch size
(yes, I know!) or (2) indent the old SetCount code in this patch.
Worse still, the old SetCount would have to be removed later during
the multi-selection listbox patch, increasing *that* one's size even
more.

It's a tradeoff between this patch's size and the multi-selection one.

> One thing you may want to do is to tidy up the allocation functions
> (in a separate patch or two).  For example InsertItem should call
> InitStorage rather than duplicate the allocation logic.  Also it would
> be nice to get rid of the HeapSize calls and simply store the length
> of the items array.  Another thing you could do would be to move to
> the wine/heap.h functions which would let you use the
> heap_realloc(NULL,...) behaviour to further simplify things.
>

I can use InitStorage in InsertItem but I didn't think it would have
anything to do with this patch. Not sure if it will help with the
multisel_expand case though since I need to pass the HEAP_ZERO_MEMORY
flag for the setcount expansion (same limitation for
heap_alloc/realloc here).

Secondly, about the common allocation. I'm not sure that's going to
work in the future because the allocation itself will be changed in
NODATA helpers (since it won't use normal item structure), so they
won't be identical anymore.

And about the HeapSize calls -- well, yeah I agree that it should be
changed, but I didn't want to overcomplicate this patchset for
changing too much stuff. I'll look into it.

>
> So this looks like
>    if (!IS_MULTISELECT(descr))
>       return index != descr->selected_item;
>    return descr->items[index].selected;
>
> and you'll extend it when nodata multiselect is optimized.
>
> Huw.

Well the reason I wanted to avoid that, was due to wanting to move all
implementation detail of nodata stuff into NODATA_* helpers. However,
if I won't use the NODATA_is_sel helper at all and just implement
everything in this function, then it doesn't make a difference I
guess.