On 11/20/18 4:40 PM, Gabriel Ivăncescu wrote:
I reduced the patch to just single-selection changes so that it still works. After these are in, the multi-selection patches should be smaller.
This should be reduced further.
Please note that the new helper functions (such as NODATA_is_sel) should remain as-is because they will be extended later for multi-selection listboxes, even if they seem redundant for now, so that I can really trim down the size of the multi-selection patches.:-)
By the way, this patch really looks larger than it is. Most of the changes are in SetCount and the helper functions. InsertItem and RemoveItem seem large, but the changes are minor -- it's mostly just indentation, so it should be simple to review.
It is large, and again for no good reason.
For example painting part:
@@ -494,7 +559,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect, RECT r; HRGN hrgn;
- if (!item)
- if (index >= descr->nb_items) {
'index' should be made unsigned throughout, unless there is a reason not to.
if (action == ODA_FOCUS) DrawFocusRect( hdc, rect );@@ -517,16 +582,19 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect, dis.hDC = hdc; dis.itemID = index; dis.itemState = 0;
if (item->selected) dis.itemState |= ODS_SELECTED;
if ( ( is_singlesel_NODATA(descr) && NODATA_is_sel(descr, index)) ||(!is_singlesel_NODATA(descr) && item->selected) )dis.itemState |= ODS_SELECTED;
This is really ugly.
if (!ignoreFocus && (descr->focus_item == index) && (descr->caret_on) && (descr->in_focus)) dis.itemState |= ODS_FOCUS; if (!IsWindowEnabled(descr->self)) dis.itemState |= ODS_DISABLED;
dis.itemData = item->data;
dis.itemData = (descr->style & LBS_NODATA) ? 0 : item->data; dis.rcItem = *rect; TRACE("[%p]: drawitem %d (%s) action=%02x state=%02x rect=%s\n",
descr->self, index, debugstr_w(item->str), action,dis.itemState, wine_dbgstr_rect(rect) );
descr->self, index, debugstr_w((descr->style & LBS_NODATA) ? NULL : item->str),action, dis.itemState, wine_dbgstr_rect(rect) ); SendMessageW(descr->owner, WM_DRAWITEM, dis.CtlID, (LPARAM)&dis); SelectClipRgn( hdc, hrgn ); if (hrgn) DeleteObject( hrgn );
Instead of spreading style checks just make sure 'item' is unset correctly.
@@ -1678,28 +1757,38 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index ) LISTBOX_InvalidateItems( descr, index );
descr->nb_items--;
- LISTBOX_DeleteItem( descr, index );
- if (!descr->nb_items) return LB_OKAY;
- /* Remove the item */
- if (is_singlesel_NODATA(descr))
- {
if (!descr->nb_items){SendMessageW(descr->self, LB_RESETCONTENT, 0, 0);return LB_OKAY;}- }
We'll need additional test for that.
- for(i = descr->nb_items - 1; i>=0; i--) LISTBOX_DeleteItem( descr, i);
- if (!is_singlesel_NODATA(descr))
for (i = descr->nb_items - 1; i >= 0; i--) LISTBOX_DeleteItem(descr, i);
Is it the same as testing for null items array?
static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count ) {
- LRESULT ret;
INT orig_num;
if (!(descr->style & LBS_NODATA)) return LB_ERR;
count = max(count, 0);
Do we have tests for that?
case LB_GETSEL: if (((INT)wParam < 0) || ((INT)wParam >= descr->nb_items)) return LB_ERR;
if (is_singlesel_NODATA(descr))return NODATA_is_sel(descr, wParam); return descr->items[wParam].selected;
Do you need this? For single selection case you'll only need to check wParam == selected_item, regardless of LBS_NODATA presence.
Regarding item storage, it's important to test what happens when multiselection styles are changed after window creation.
Speaking of optimizations, having additional field for a number of selected items could improve LB_GETSELCOUNT and LB_GETSELITEMS.