On Wed, Nov 21, 2018 at 4:51 PM Huw Davies huw@codeweavers.com wrote:
+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().
Sure. I was just trying to separate all of the NODATA internal stuff into their own helpers.
+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.
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.
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.