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

[Huw Davies]

On Wed, Nov 21, 2018 at 03:38:47PM +0200, Gabriel Ivăncescu wrote:
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=32374
> Signed-off-by: Gabriel Ivăncescu
>
> SetCount's count was changed to UINT because otherwise it will fail to pass
> the tests (next patch).
>
> dlls/comctl32/listbox.c | 269 +++++++++++++++++++++++++++++-----------
>  1 file changed, 194 insertions(+), 75 deletions(-)

This is getting better, but this patch is still way too big and
does things that are unnecessary.
> diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> index 0638cdb..059ac90 100644
> --- a/dlls/comctl32/listbox.c
> +++ b/dlls/comctl32/listbox.c
> @@ -19,7 +19,7 @@
> - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> -
> - TODO:
> -
> -
> - LBS_NODATA
> -
> -
> - LBS_NODATA for multi-selection listboxes
> */
>
> #include <string.h>
> @@ -125,8 +125,75 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE;
>
> static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect );
>
> +/***********************************************************************
> -
> -
>       Helper functions for LBS_NODATA listboxes
> -
> -
> -
> - Since LBS_NODATA listboxes can be extremely large, we need to store
> -
> - them with minimal overhead, both for performance and memory usage.
> -
> -
> -
> - In all cases, it is important to note that descr->items must never be
> -
> - dereferenced directly with LBS_NODATA, outside of these helpers.
> -
> -
> -
> - For single-selection listboxes, we store literally no data for items,
> -
> - and thus descr->items will always be NULL in this case.
> -
> -
> -
> - FIXME: Multi-selection listboxes are not implemented yet for LBS_NODATA.
> -
> -
> - */
>
> +static BOOL is_singlesel_NODATA(const LB_DESCR *descr)
> +{
> - return (descr->style & LBS_NODATA) &&
> -
>       !(descr->style & (LBS_MULTIPLESEL | LBS_EXTENDEDSEL));
>
> +}
> -
>
> +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().
> -
>
> +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.

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.
> -
>
> +static void NODATA_multisel_shrink(LB_DESCR *descr, UINT orig_num)
> +{
> - LB_ITEMDATA *p = descr->items;
> - UINT sz = descr->nb_items * sizeof(*p);
> - UINT orig_sz = orig_num * sizeof(*p);
> -
> - /* Shrink the array if needed */
> - if (sz + LB_ARRAY_GRANULARITY * sizeof(*p) * 2 < HeapSize(GetProcessHeap(), 0, p))
> - {
> -
>    UINT rnd_sz = sz +  LB_ARRAY_GRANULARITY * sizeof(*p) - 1;
> -
>    rnd_sz -= rnd_sz % (LB_ARRAY_GRANULARITY * sizeof(*p));
> -
>    if ((p = HeapReAlloc(GetProcessHeap(), 0, p, rnd_sz)))
> -
>    {
> -
>        descr->items = p;
> -
>        orig_sz = rnd_sz;
> -
>    }
> - }
> - memset(&p[sz / sizeof(*p)], 0, orig_sz - sz);
>
> +}
> -
> -
> -
>
> static BOOL is_item_selected(LB_DESCR *descr, UINT index)
>  {
> - if (is_singlesel_NODATA(descr))
> -
>    return NODATA_is_sel(descr, index);
> return descr->items[index].selected;
>
> }

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.