[PATCH 1/2] comctl32/listbox: Update the size in SetColumnWidth unconditionally
When the listbox receives a WM_WINDOWPOSCHANGED message and calls DefWindowProc on it, which then ends up sending a WM_SIZE to the window, some applications swallow that up and won't pass the WM_SIZE to the listbox. However, they do pass a LB_SETCOLUMNWIDTH, even when the column width did not change at all. So we have to unconditionally update the size itself (not just the page) when we receive that message. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=22440 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/comctl32/listbox.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c index b3491bb..b3719a6 100644 --- a/dlls/comctl32/listbox.c +++ b/dlls/comctl32/listbox.c @@ -1260,10 +1260,12 @@ static LRESULT LISTBOX_SetHorizontalExtent( LB_DESCR *descr, INT extent ) */ static LRESULT LISTBOX_SetColumnWidth( LB_DESCR *descr, INT width) { - if (width == descr->column_width) return LB_OKAY; - TRACE("[%p]: new column width = %d\n", descr->self, width ); + /* update the SIZE unconditionally, because some applications swallow + WM_SIZE from DefWindowProc and only send LB_SETCOLUMNWIDTH, even + when the column width doesn't even change (but its size does) */ + TRACE("[%p]: new column width = %d\n", descr->self, width); descr->column_width = width; - LISTBOX_UpdatePage( descr ); + LISTBOX_UpdateSize(descr); return LB_OKAY; } -- 1.9.1
When the listbox receives a WM_WINDOWPOSCHANGED message and calls DefWindowProc on it, which then ends up sending a WM_SIZE to the window, some applications swallow that up and won't pass the WM_SIZE to the listbox. However, they do pass a LB_SETCOLUMNWIDTH, even when the column width did not change at all. So we have to unconditionally update the size itself (not just the page) when we receive that message. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=22440 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/user32/listbox.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/dlls/user32/listbox.c b/dlls/user32/listbox.c index 991ab87..fbed95c 100644 --- a/dlls/user32/listbox.c +++ b/dlls/user32/listbox.c @@ -1265,10 +1265,12 @@ static LRESULT LISTBOX_SetHorizontalExtent( LB_DESCR *descr, INT extent ) */ static LRESULT LISTBOX_SetColumnWidth( LB_DESCR *descr, INT width) { - if (width == descr->column_width) return LB_OKAY; - TRACE("[%p]: new column width = %d\n", descr->self, width ); + /* update the SIZE unconditionally, because some applications swallow + WM_SIZE from DefWindowProc and only send LB_SETCOLUMNWIDTH, even + when the column width doesn't even change (but its size does) */ + TRACE("[%p]: new column width = %d\n", descr->self, width); descr->column_width = width; - LISTBOX_UpdatePage( descr ); + LISTBOX_UpdateSize(descr); return LB_OKAY; } -- 1.9.1
On 08/20/2018 06:49 PM, Gabriel Ivăncescu wrote:
When the listbox receives a WM_WINDOWPOSCHANGED message and calls DefWindowProc on it, which then ends up sending a WM_SIZE to the window, some applications swallow that up and won't pass the WM_SIZE to the listbox. However, they do pass a LB_SETCOLUMNWIDTH, even when the column width did not change at all. So we have to unconditionally update the size itself (not just the page) when we receive that message.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=22440 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/user32/listbox.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/dlls/user32/listbox.c b/dlls/user32/listbox.c index 991ab87..fbed95c 100644 --- a/dlls/user32/listbox.c +++ b/dlls/user32/listbox.c @@ -1265,10 +1265,12 @@ static LRESULT LISTBOX_SetHorizontalExtent( LB_DESCR *descr, INT extent ) */ static LRESULT LISTBOX_SetColumnWidth( LB_DESCR *descr, INT width) { - if (width == descr->column_width) return LB_OKAY; - TRACE("[%p]: new column width = %d\n", descr->self, width ); + /* update the SIZE unconditionally, because some applications swallow + WM_SIZE from DefWindowProc and only send LB_SETCOLUMNWIDTH, even + when the column width doesn't even change (but its size does) */ + TRACE("[%p]: new column width = %d\n", descr->self, width); descr->column_width = width; - LISTBOX_UpdatePage( descr ); + LISTBOX_UpdateSize(descr); return LB_OKAY; }
Hi, Gabriel. Thanks for the patches. I think there's more problems with resizing, that could affect how this one should be fixed. For example if LBS_NOINTEGRALHEIGHT is not set it's possible to get last item partially visible. On Windows initial window size is also adjusted to be a multiple of item height, so setting column width does not have to resize anything. When control does not have WM_SIZE is integral height still enforced? Could you look into that?
On Tue, Aug 21, 2018 at 1:25 PM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
On 08/20/2018 06:49 PM, Gabriel Ivăncescu wrote:
When the listbox receives a WM_WINDOWPOSCHANGED message and calls DefWindowProc on it, which then ends up sending a WM_SIZE to the window, some applications swallow that up and won't pass the WM_SIZE to the listbox. However, they do pass a LB_SETCOLUMNWIDTH, even when the column width did not change at all. So we have to unconditionally update the size itself (not just the page) when we receive that message.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=22440 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/user32/listbox.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/dlls/user32/listbox.c b/dlls/user32/listbox.c index 991ab87..fbed95c 100644 --- a/dlls/user32/listbox.c +++ b/dlls/user32/listbox.c @@ -1265,10 +1265,12 @@ static LRESULT LISTBOX_SetHorizontalExtent( LB_DESCR *descr, INT extent ) */ static LRESULT LISTBOX_SetColumnWidth( LB_DESCR *descr, INT width) { - if (width == descr->column_width) return LB_OKAY; - TRACE("[%p]: new column width = %d\n", descr->self, width ); + /* update the SIZE unconditionally, because some applications swallow + WM_SIZE from DefWindowProc and only send LB_SETCOLUMNWIDTH, even + when the column width doesn't even change (but its size does) */ + TRACE("[%p]: new column width = %d\n", descr->self, width); descr->column_width = width; - LISTBOX_UpdatePage( descr ); + LISTBOX_UpdateSize(descr); return LB_OKAY; }
Hi, Gabriel.
Thanks for the patches. I think there's more problems with resizing, that could affect how this one should be fixed. For example if LBS_NOINTEGRALHEIGHT is not set it's possible to get last item partially visible. On Windows initial window size is also adjusted to be a multiple of item height, so setting column width does not have to resize anything.
When control does not have WM_SIZE is integral height still enforced?
Could you look into that?
Hello Nikolay, In LISTBOX_UpdateSize, the check for lack of LBS_NOINTEGRALHEIGHT is skipped (since Total Commander does have it set in this case). Though, shouldn't that handle it properly if LBS_NOINTEGRALHEIGHT is not set? It seems UpdateSize has code to deal with that. The second part of that check for LBS_OWNERDRAWVARIABLE is always going to be true in such situation, since SetColumnWidth only applies to multi-column listboxes and they are incompatible with that style. I don't think that calling UpdateSize can do any actual "harm", except just recalculating some things (based on width & height). Unfortunately I don't know of an application without LBS_NOINTEGRALHEIGHT to test, so I'd have to artificially inject it in Total Commander but I don't know if that's a proper way to test it... Maybe UpdateSize itself needs fixing when LBS_NOINTEGRALHEIGHT is not set? (but I feel it should be a different bug) This patch just handles a quirk of some applications and apparently Windows. I don't know what Windows does exactly, but I think it updates the width & height when it receives a LB_SETCOLUMNWIDTH as well, otherwise it wouldn't work. The important part of UpdateSize that has to get called unconditionally is the GetClientRect( descr->self, &rect ); and updating the width & height that (possibly) changed even if the column width did not. After that it calls UpdatePage anyway. I mean I could've called GetClientRect myself and do that, but I thought reusing UpdateSize was a better idea :) Since it also handles LBS_NOINTEGRALHEIGHT unset. In summary: the check of if(descr->column_width == width) is logically valid but it results in the bug, because SetColumnWidth apparently gets called instead of WM_SIZE *even* if the column's width does *not* change at all, which is obviously a quirk that Windows seems to handle and probably why applications make use of it. So the width & height themselves have to be updated to match what WM_SIZE would have done (if they did, there's no way to know without calling UpdateSize or replicating that code...). So what happens is something like this: WM_WINDOWPOSCHANGED in listbox calls DefWindowProc -> eventually sends WM_MOVE (passed correctly to listbox) followed by WM_SIZE -> app's window proc hijacks WM_SIZE and does not pass WM_SIZE to listbox, instead it sends LB_SETCOLUMNWIDTH, *even* if the width did not change at all (so descr->column_width == width in many such cases). However, the width & height did change, since it completely replaces any such WM_SIZE with it, that's why it has to be updated without regard for column_width. But, if you had something else in mind, please elaborate, I feel like I'm missing something obvious... or I was too verbose. This quirk surprised me at least. Another way to "fix" this would be to call UpdateSize from WM_WINDOWPOSCHANGED itself, but then we'd call it again when WM_SIZE is passed later (in rare cases for multi-column listboxes), and I don't think that's a good idea as it will also mess with non-multi-column listboxes... and feels more hackish to me. But maybe it's better?
Actually, I thought of something else: why not just add a check in LISTBOX_UpdateSize itself to see if width and height have changed at all? And if they haven't, just return early? This should alleviate any issues since in this case, calling UpdateSize from SetColumnWidth when the size itself hasn't changed shouldn't do a thing (it will return early from it and not invalidate anything). If that's ok, I will prepare a patch for that. (obviously, it will be a separate patch, this one is still valid in this case)
Now I had some time to think about this (was tracking another bug ;) )... I think if I go with this idea, I'll have to revamp the patch and not check that within UpdateSize itself, since UpdatePage may have to be called even if width & height don't change. To avoid calling GetClientRect twice, I thought of factoring it like this: First, take out the GetClientRect from UpdateSize (and add two parameters to it, the width & height). Of course, at WM_SIZE do the GetClientRect portion and pass that to UpdateSize, so it will be identical to before (in behavior). Second, in SetColumnWidth, use GetClientRect and check if width & height changed at all; if they did, set the column_width and call UpdateSize. If they did not, then check if column_width changed and call UpdatePage (i.e. the original code's behavior). Would that be better? Or am I overcomplicating it and the current patch is fine here? On Tue, Aug 21, 2018 at 5:09 PM, Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
Actually, I thought of something else: why not just add a check in LISTBOX_UpdateSize itself to see if width and height have changed at all? And if they haven't, just return early?
This should alleviate any issues since in this case, calling UpdateSize from SetColumnWidth when the size itself hasn't changed shouldn't do a thing (it will return early from it and not invalidate anything).
If that's ok, I will prepare a patch for that. (obviously, it will be a separate patch, this one is still valid in this case)
participants (2)
-
Gabriel Ivăncescu -
Nikolay Sivov