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

[Gabriel Ivăncescu]

On Wed, Nov 21, 2018 at 7:04 PM Huw Davies <huw at codeweavers.com> wrote:
>
> 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.
>

Yeah, that's exactly what I was doing with the multisel expand
function, except for using a common allocator. I guess I can move it
to before this patch somehow.

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

The only difference I can see is that the custom allocator deals with
sizes in bytes, instead of sizes in "number of items", otherwise
there's no way to share it. I'll try something like that, hope that's
fine.

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

Well I was trying to abstract the operations also within the helpers
(so helpers within helpers!) for NODATA :-) But nevermind.