On Wed, Nov 21, 2018 at 10:42 PM Nikolay Sivov nsivov@codeweavers.com wrote:
On 11/21/18 11:30 PM, Gabriel Ivăncescu wrote:
On Wed, Nov 21, 2018 at 5:21 PM Nikolay Sivov nsivov@codeweavers.com wrote:
Please use message sequence for this one, see how ok_sequence() is used.
I'm sorry I missed your reply, I'll resend it fixed tomorrow (please review rest of patches if you can so I can fix more at once).
I'll try, but please keep it around 5 patches in a batch.
I see patch 10 introduces NODATA_* helpers again, and I already explained that we don't need that.
I thought you meant just the NODATA_is_sel helper, which was removed.
The NODATA helpers will be very useful for maintenance and to ease changes when implementing multi-selection. Consider the fact that we'll need at least NODATA behavior for Expand, Shrink, InsertItem, RemoveItem, GetSelItems, SelectItemRange, and perhaps GetSelCount.
The helpers allow to (1) comment everything above them so their purpose and behavior is clear and (2) allow very easy maintenance and changes to the storage since all you have to care about are the helpers.
For example, suppose we want to modify the byte array of multi-selection into the range-based storage as you initially suggested (as per ListView). Or perhaps we want to change it to a bit array, which is probably simpler to implement, and provides nice gains and much lower memory usage. Either way, it's easy to make these changes.
To do that, we simply change the relevant NODATA helpers in a new patch, and that's it. The patch will be clean and there won't be side-effects such as "did I forget to change X?" and you won't have to look through the whole listbox code to figure that out. It keeps the NODATA internal implementation (storage etc) separate so that it can be cleanly modified without affecting the rest of the code. It's all done to make changing it much easier.
In short, I think the NODATA helpers massively ease maintenance and future changes to the storage layout. These are just first step to make the multi-selection patch smaller. This is another reason the allocators are done also in NODATA helpers, because obviously if we later go by the ListView route of storing ranges of selections, it will have to be completely separate from the normal allocation (works totally differently).
What is so bad about them?