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