Hi,
I think this was rejected too hastily. Sorry if the analysis will be a little verbose, but sometimes it's really hard to convince...
I don't think SBS_SIZEBOX is a net gain in code simplicity (on the contrary) if you take the entire patchset into account. I tried it first and gave up on it, but maybe I'm missing something...
I mean, what are the problems with the current code? As far as I am aware (please elaborate if I'm missing something), the possible ones:
(1) is painting the grip manually an issue? (2) is WM_NCHITTEST an issue? (3) is WM_NCCALCSIZE an issue?
I can't see anything else compared to a SBS_SIZEBOX implementation, so let's analyze the above 3 and understand why they are needed *even* with SBS_SIZEBOX. But first, please take a look at patch 4/4 (162405) if you haven't, to see what kind of code would need to be at the end of the series. That's very important for my point below.
Now, let's assume that SBS_SIZEBOX was used for the grip on patch 2/4 (presumably that's why it was rejected) and look at the 3 issues again, with the patch 4/4 in mind:
(1) painting the grip manually will *still* have to be done because the grip needs to be flipped vertically in patch 4/4. No gain in simplicity.
(2) WM_NCHITTEST will *still* have to be done manually because of the vertical flip (HTTOPRIGHT instead) *and* the fact the grip acts like a square even when it's a triangle, to match Windows. No gain.
(3) WM_NCCALCSIZE can be removed. However, since with SBS_SIZEBOX when the scrollbar is visible, we can't just simply set the scroll/grip window to the listbox height in update_listbox_size, but instead have to special-case the flip to position both properly in that case... it would simply move the logic to update_listbox_size instead of WM_NCCALCSIZE. No real gain in simplicity.
Meanwhile, using SBS_SIZEBOX means adding yet another window to keep track of, initialize, and then position in update_listbox_size with the rest. No matter how I look at it, it will add *more* code, not less, after patch 4/4. But again, maybe I am missing something, and if so please elaborate on why you think using SBS_SIZEBOX is a better idea.
I'd appreciate if I didn't have to actually waste time changing the code to end up with slightly more complex code, which is the opposite goal of what was intended...
Thanks, Gabriel
On 4/12/19 9:11 PM, Marvin wrote:
Thank you for your contribution to Wine!
This is an automated notification to let you know that your patch has been reviewed and its status set to "Rejected".
This means that the patch has been rejected by a reviewer. You should have received a mail explaining why it was rejected. You need to fix the issue and resend the patch, or if you are convinced that your patch is good as is, you should reply to the rejection message with your counterarguments.
If you do not understand the reason for this status, disagree with our assessment, or are simply not sure how to proceed next, please ask for clarification by replying to this email.
-
Gabriel Ivăncescu