[PATCH 1/4] shell32/autocomplete: Implement the listbox scrollbar manually
Gabriel Ivăncescu
gabrielopcode at gmail.com
Sat Apr 13 06:41:29 CDT 2019
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.
>
More information about the wine-devel
mailing list