[PATCH v4 6/7] comctl32/listbox: Use a helper to set the selected item state

[Huw Davies]

On Wed, Feb 13, 2019 at 01:11:35PM +0200, Gabriel Ivăncescu wrote:
> On 2/13/19 12:00 PM, Huw Davies wrote:
> > On Tue, Feb 12, 2019 at 03:47:02PM +0200, Gabriel Ivăncescu wrote:
> > > Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
> > > ---
> > > 
> > > is_singlesel_NODATA is needed otherwise it fails the tests for multi-selection
> > > nodata, because it's not implemented yet.
> > > 
> > >   dlls/comctl32/listbox.c | 20 +++++++++++++++-----
> > >   1 file changed, 15 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > > index f99406a..706a0b8 100644
> > > --- a/dlls/comctl32/listbox.c
> > > +++ b/dlls/comctl32/listbox.c
> > > @@ -127,6 +127,11 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE;
> > >   static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect );
> > > +static BOOL is_singlesel_NODATA(const LB_DESCR *descr)
> > > +{
> > > +    return (descr->style & (LBS_NODATA | LBS_MULTIPLESEL | LBS_EXTENDEDSEL)) == LBS_NODATA;
> > > +}
> > > +
> > >   static ULONG_PTR get_item_data( const LB_DESCR *descr, UINT index )
> > >   {
> > >       return (descr->style & LBS_NODATA) ? 0 : descr->items[index].data;
> > > @@ -170,6 +175,11 @@ static BOOL is_item_selected( const LB_DESCR *descr, UINT index )
> > >       return descr->items[index].selected;
> > >   }
> > > +static void set_item_selected_state(LB_DESCR *descr, UINT index, BOOL state)
> > > +{
> > > +    if (!is_singlesel_NODATA(descr)) descr->items[index].selected = state;
> > > +}
> > 
> > To mirror is_item_selected() I'd suggest only setting selected in the multiselect case,
> > so this would become:
> > 
> >      if (IS_MULTISELECT(descr)) descr->items[index].selected = state;
> > 
> > Huw.
> > 
> 
> I just realized that this would become a no-op in _SetSelection, and a
> redundant check in the other (because it's always multi-select there).
> 
> So my question is, should I just get rid of the calls entirely in the
> _SetSelection function.

Let's still call them.

> and keep the code as-is (for now) in the
> _SelectItemRange? (the latter will change when multi-select nodata listbox
> gets implemented, so this helper is unneeded for now)

And use the helpers here.  Note also you should use is_item_selected()
instead of reading ->items[i].selected directly.

Huw.