Re: [PATCH 1/4] shell32/autocomplete: Implement the listbox scrollbar manually

[Gabriel Ivăncescu]

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.