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

[Huw Davies]

On Wed, Nov 21, 2018 at 05:47:06PM +0200, Gabriel Ivăncescu wrote:
> On Wed, Nov 21, 2018 at 4:51 PM Huw Davies <huw at codeweavers.com> wrote:
> > > +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.

Like I've already said, I think you want to change SetCount in an
earlier patch to allocate the block in one go - it would call into
some common allocator function also called by InitStorage.  This patch
would then simply skip the call to the allocator.

> > 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.

Plewase do this.  Part of the reason this patch is proving difficult
is the messy allocation scheme that already exists.  Let's tidy
that up first.  Everything should fall back to a common allocator,
once that's done, it becomes easy to alter the size that that
will allocate if we shift to using a byte array for the nodata
multiselect case.


> >
> > 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.

We're trying to stop you pulling the nodata stuff into separate
functions, but rather use helpers to abstract certain operations
(like checking the selected state and allocation).  Those helpers
will then need to worry about the details.

Huw.