list view completely mangled in WINE

[Tobias Burnus]

Hello,

in Crystal Impact's Diamond the list view is completely mangled and only 
   barely recognizable as list view.

Result in WINE:
http://www.physik.fu-berlin.de/~tburnus/wine/diamond_list_wine.png

Result in Windows:
http://www.physik.fu-berlin.de/~tburnus/wine/diamond_list.png

I'd be really pleased if someone could look at the problem.

The demo version of the program is free and available from
http://www.crystalimpact.com/download/diamond/demo/dm2demo.exe

(In addition to this list-view problem the application does off-screen 
rendering which is not supported by WINE; one needs to disable rendering
in order use the program. See WINE bug
http://bugs.winehq.com/show_bug.cgi?id=2320 )

Tobias

[Uwe Bonnes]

> > > > > "Tobias" == Tobias Burnus  writes:

Tobias> Hello, in Crystal Impact's Diamond the list view is completely
    Tobias> mangled and only barely recognizable as list view.

Tobias> Result in WINE:
    Tobias> http://www.physik.fu-berlin.de/~tburnus/wine/diamond_list_wine.png

Tobias> Result in Windows:
    Tobias> http://www.physik.fu-berlin.de/~tburnus/wine/diamond_list.png

Consider running with native comctl32 as a workaround. Wine comctl32 still
has a long way to go...

...

Bye

[Dimitrie O. Paun]

On Fri, Jul 30, 2004 at 05:26:57PM +0200, Tobias Burnus wrote:
> Hello,
>
> in Crystal Impact's Diamond the list view is completely mangled and only 
>   barely recognizable as list view.

Tobias,

I'll try to look into it. I have been however immensely busy
lately, so please remind me if I don't get back to you soon.

[Filip Navara]

Tobias Burnus wrote:
[snip]

Can you try the attached patch please?

Regards,
Filip

Changelog:
- Don't update infoPtr->dwStyle in LISTVIEW_WindoProc. It's already
  handled in LISTVIEW_StyleChanged and LISTVIEW_Create processing.
- Fix TOOLBAR_DrawMasked to correctly use image list mask.

Index: comctl32/toolbar.c
===================================================================
--- comctl32/toolbar.c	(revision 1)
+++ comctl32/toolbar.c	(working copy)
@@ -658,8 +658,7 @@

/* Create src image */
         hdcImage = CreateCompatibleDC(hdc);
-        hbmImage = CreateBitmap(cx, cy, GetDeviceCaps(hdc,PLANES),
-                                GetDeviceCaps(hdc,BITSPIXEL), NULL);
+        hbmImage = CreateCompatibleBitmap(hdc, cx, cy);
         SelectObject(hdcImage, hbmImage);
         ImageList_DrawEx(himl, index, hdcImage, 0, 0, cx, cy,
                          RGB(0xff, 0xff, 0xff), RGB(0,0,0), ILD_NORMAL);
@@ -670,8 +669,8 @@
         SelectObject(hdcMask, hbmMask);

/* Remove the background and all white pixels */
-        SetBkColor(hdcImage, ImageList_GetBkColor(himl));
-        BitBlt(hdcMask, 0, 0, cx, cy, hdcImage, 0, 0, SRCCOPY);
+        ImageList_DrawEx(himl, index, hdcMask, 0, 0, cx, cy,
+                         RGB(0xff, 0xff, 0xff), RGB(0,0,0), ILD_MASK);
         SetBkColor(hdcImage, RGB(0xff, 0xff, 0xff));
         BitBlt(hdcMask, 0, 0, cx, cy, hdcImage, 0, 0, NOTSRCERASE);

Index: comctl32/listview.c
===================================================================
--- comctl32/listview.c	(revision 1)
+++ comctl32/listview.c	(working copy)
@@ -117,7 +117,6 @@
  *   -- LVM_GETISEARCHSTRINGW, LVM_GETISEARCHSTRINGA
  *   -- LVM_GETTILEINFO, LVM_SETTILEINFO
  *   -- LVM_GETTILEVIEWINFO, LVM_SETTILEVIEWINFO
- *   -- LVM_GETTOOLTIPS, LVM_SETTOOLTIPS
  *   -- LVM_GETUNICODEFORMAT, LVM_SETUNICODEFORMAT
  *   -- LVM_GETVIEW, LVM_SETVIEW
  *   -- LVM_GETWORKAREAS, LVM_SETWORKAREAS
@@ -8782,11 +8781,6 @@
   if (!infoPtr && (uMsg != WM_CREATE))
     return DefWindowProcW(hwnd, uMsg, wParam, lParam);

-  if (infoPtr)
-  {
-    infoPtr->dwStyle = GetWindowLongW(hwnd, GWL_STYLE);
-  }
-
   switch (uMsg)
   {
   case LVM_APPROXIMATEVIEWRECT:

[Tobias Burnus]

Hello,

Filip Navara wrote:
> Can you try the attached patch please?

The patch improves the situation a lot! Even Save List works now!
Thanks!

Before:
http://www.physik.fu-berlin.de/~tburnus/wine/diamond_list_wine.png
Afterwards:
http://www.physik.fu-berlin.de/~tburnus/wine/diamond_list_wine2.png

The only problem I still have is the horizontal scrolling in listviews.
If I reduce the window size, horizontal scrollbars appear and everything 
looks fine
(http://www.physik.fu-berlin.de/~tburnus/wine/diamond_list_wine3a.png).

Now, I click on the scrollbar to scroll to the rightmost columns; while 
the header is correct, the list itself contains the FIRST columns not 
the columns which belong to the column headers
http://www.physik.fu-berlin.de/~tburnus/wine/diamond_list_wine3b.png

Regards,

Tobias

[Filip Navara]

Tobias Burnus wrote:
> The patch improves the situation a lot! Even Save List works now!
> Thanks!

Great!

[snip]
> Now, I click on the scrollbar to scroll to the rightmost columns; 
> while the header is correct, the list itself contains the FIRST 
> columns not the columns which belong to the column headers
> http://www.physik.fu-berlin.de/~tburnus/wine/diamond_list_wine3b.png

Should be fixed in the attached patch, but there is still one problem 
with refreshing of the list view after shrinking the column headers. 
Unfortunetly I don't have time to fix that.

Regards,
Filip

[Tobias Burnus]

Hello,

Tobias Burnus wrote:
> The patch improves the situation a lot! Even Save List works now!
> Thanks!

I have another strangeness with multi-select lists (e.g. the lists in 
Diamond but this might affect other lists as well):

* I go to a line, press shift and then the arrow keys.
If I use arrow down, I can select as many lines as I want.
If I use arrow up, I can only select two lines: The line I'm in and the 
one below, the others further below are unselected as I move upwards.

* Multi-select with ctrl doesn't work using the keyboard.
Select a line, press ctrl, move up/down to another line and press space.
Expected: This line is selected as well.
Actual result: Nothing happens

(Not a bug, but a missing handy feature: In Windows,  double clicking 
between two column headers (cursor: <-|->) adjusts the column-width so 
that everything fits. In WINE nothing happens. (Work-around: go to the 
same place and adjust manually.))

Regards,

Tobias,
who hopes that the submitted patches get applied to CVS.

[Dimitrie O. Paun]

On Sat, Jul 31, 2004 at 05:52:21AM +0200, Filip Navara wrote:
> Tobias Burnus wrote:
> [snip]
>
> Can you try the attached patch please?
>
> Changelog:
> -
>
> Don't update infoPtr->dwStyle in LISTVIEW_WindoProc. It's already
> handled in LISTVIEW_StyleChanged and LISTVIEW_Create processing.
> -
>
> if (infoPtr)
> -
>
> {
> -
>
> infoPtr->dwStyle = GetWindowLongW(hwnd, GWL_STYLE);
> -
>
> }
> -
>
> switch (uMsg)
>  {
>  case LVM_APPROXIMATEVIEWRECT:

Can you please explain why updating it is a problem? Yes, we may be
doing more work than needed, but at most it should be a no-op.
In fact, we used to call 'GetWindowLongW(hwnd, GWL_STYLE)' every
time we needed to get to the dwStyle, so updating it in there
should be OK.

BTW, I've tried to install the demo, but the installer dies on my
version of wine. Does it work for you, or did you use Windows for
the installation?

[Filip Navara]

Dimitrie O. Paun wrote:
> Can you please explain why updating it is a problem? Yes, we may be
> doing more work than needed, but at most it should be a no-op.
> In fact, we used to call 'GetWindowLongW(hwnd, GWL_STYLE)' every
> time we needed to get to the dwStyle, so updating it in there
> should be OK.

Unfortunetly I can't explain it, because I don't understand it myself. I 
have inserted debug messages on all the places where the listview 
display mode can be changed  and none of them appeared. After going once 
more through the whole code I found this. Obviously it seemed redundant 
and the only reason for doing that was workarounding problems with 
changed WS_VSCROLL and WS_HSCROLL styles (which correctly don't recieve 
WM_STYLECHANGED notifications from the scrolling code). So I tried 
removing it and ... whoa ... to my surprise it started to work.
> BTW, I've tried to install the demo, but the installer dies on my
> version of wine. Does it work for you, or did you use Windows for
> the installation?

Yes, I installed it on Windows and even most of done most of the testing 
on Windows with Wine COMCTL32. Before sending the patch to wine-patches 
I checked if it works as desired on Wine...

- Filip

[Dimitrie O. Paun]

On Tue, Aug 03, 2004 at 01:42:28AM +0200, Filip Navara wrote:
> Unfortunetly I can't explain it, because I don't understand it myself. I

In which case I must object to the patch :( It seems we're just hiding
a problem in a very convoluted way, don't you think?
> have inserted debug messages on all the places where the listview 
> display mode can be changed  and none of them appeared. After going once 
> more through the whole code I found this. Obviously it seemed redundant 
> and the only reason for doing that was workarounding problems with 
> changed WS_VSCROLL and WS_HSCROLL styles (which correctly don't recieve 
> WM_STYLECHANGED notifications from the scrolling code). So I tried 
> removing it and ... whoa ... to my surprise it started to work.

The problem is that WM_STYLECHANGED is sometimes sent, sometimes not,
and it seems so much easier to just update it every time we come in
so we can be sure we're using the latest value, as we should. It's
redundant, I agree, but it's neglijable in terms of performance so
I've put it in due to it's simplicity. This way the dwStyle has a
very simple to understand semantics. In fact, I've meant to remove
it eventually, just like you did, but only as an optimization. However,
doing so to fix a bug in a way that we don't understand worries me
greatly. I'd rather keep it in, and fix the problem.

[Filip Navara]

Dimitrie O. Paun wrote:
> On Tue, Aug 03, 2004 at 01:42:28AM +0200, Filip Navara wrote:
> > Unfortunetly I can't explain it, because I don't understand it 
> > myself. I
>
> In which case I must object to the patch :( It seems we're just hiding
> a problem in a very convoluted way, don't you think?

Well, I agree, the patch shouldn't go in, but I don't have currently 
time to investigate the issue behind this. Maybe in a week or so. If you 
want to give it a try, feel free to do so...

- Filip

[Filip Navara]

Dimitrie O. Paun wrote:
[snip]
> The problem is that WM_STYLECHANGED is sometimes sent, sometimes not,
> and it seems so much easier to just update it every time we come in
> so we can be sure we're using the latest value, as we should. It's
> redundant, I agree, but it's neglijable in terms of performance so
> I've put it in due to it's simplicity. This way the dwStyle has a
> very simple to understand semantics. In fact, I've meant to remove
> it eventually, just like you did, but only as an optimization. However,
> doing so to fix a bug in a way that we don't understand worries me
> greatly. I'd rather keep it in, and fix the problem.

I did another test. I intserted a code into the ListView window 
procedure to print a message every time the internal dwStyle member 
doesn't match the one returned by GetWindowLong with GWL_STYLE. To my 
little surprise they were different right from the beginning (even for 
WM_CREATE message). The one returned by GetWindowLong didn't contain the 
listview specific flags, WS_VSCROLL/WS_HSCROLL sometimes didn't match 
(as I explained why) and for the WS_CREATE message the WS_VISIBLE flag 
was different. I searched my local MSDN copy in order to find why this 
happens and the only relevant article I found is this:
http://support.microsoft.com/default.aspx?scid=kb;EN-US;Q83366. The 
"SUMMARY" part of the article sort of explains why this happens (at 
least according to my understanding) and so I think the patch is OK. Do 
you agree?

- Filip

[Dimitrie O. Paun]

On Tue, Aug 03, 2004 at 03:17:27AM +0200, Filip Navara wrote:
> I did another test. I intserted a code into the ListView window 
> procedure to print a message every time the internal dwStyle member 
> doesn't match the one returned by GetWindowLong with GWL_STYLE. To my 
> little surprise they were different right from the beginning (even for 
> WM_CREATE message). The one returned by GetWindowLong didn't contain the 
> listview specific flags,

If so, how did the listview work at all until now?!?
> WS_VSCROLL/WS_HSCROLL sometimes didn't match 
> (as I explained why) and for the WS_CREATE message the WS_VISIBLE flag 
> was different. I searched my local MSDN copy in order to find why this 
> happens and the only relevant article I found is this:
> http://support.microsoft.com/default.aspx?scid=kb;EN-US;Q83366. The

Good stuff, maybe we should link to it from WineHQ
> "SUMMARY" part of the article sort of explains why this happens (at 
> least according to my understanding) and so I think the patch is OK. Do 
> you agree?

Almost. If it does go in though, we need to:
  -- fix a few more places where we look at WS_[VH]SCROLL
  -- understand why did the listview work at all if the LVS_ stuff was missing
  -- do an audit (or add a Janitorial task) to audit and fix the
     other controls as well.

(of course, only the 1st point is really needed for the patch to go in,
 but the other two would be nice to have).

[Filip Navara]

Dimitrie O. Paun wrote:
[snip]
> Almost. If it does go in though, we need to:
>  -- fix a few more places where we look at WS_[VH]SCROLL

Ok, I have that done in my local tree and will send it tonight propably.
> -- understand why did the listview work at all if the LVS_ stuff was missing

I tried to run RegEdit with the same version of COMCTL32 and it 
preserved the listview specific flags. Maybe somebody with access to MFC 
source code (that is used by the Diamond application) can find out more. 
The application calls "CCtrlView::CCtrlView(WC_LISTVIEW, WS_VISIBLE | 
WS_CHILD | WS_BORDER)", so it must propably inject the listview flags 
later on, but I don't know enough about MFC internals to tell you how it 
does that (the window procedure of the list view is subclassed BTW).

- Filip

[Robert Shearman]

Dimitrie O. Paun wrote:
> On Tue, Aug 03, 2004 at 01:42:28AM +0200, Filip Navara wrote:
> > have inserted debug messages on all the places where the listview 
> > display mode can be changed  and none of them appeared. After going once 
> > more through the whole code I found this. Obviously it seemed redundant 
> > and the only reason for doing that was workarounding problems with 
> > changed WS_VSCROLL and WS_HSCROLL styles (which correctly don't recieve 
> > WM_STYLECHANGED notifications from the scrolling code). So I tried 
> > removing it and ... whoa ... to my surprise it started to work.
>
> The problem is that WM_STYLECHANGED is sometimes sent, sometimes not,
> and it seems so much easier to just update it every time we come in
> so we can be sure we're using the latest value, as we should.

The toolbar control works fine just using WM_STYLECHANGED to notify when 
it needs to update its internal state and it seems to send the message 
for all style changes that it needs (it doesn't depend on WS_ styles).
> It's
> redundant, I agree, but it's neglijable in terms of performance so
> I've put it in due to it's simplicity. This way the dwStyle has a
> very simple to understand semantics. In fact, I've meant to remove
> it eventually, just like you did, but only as an optimization. However,
> doing so to fix a bug in a way that we don't understand worries me
> greatly. I'd rather keep it in, and fix the problem.

Rob

[Tobias Burnus]

Hello,

Dimitrie O. Paun wrote:
> BTW, I've tried to install the demo, but the installer dies on my
> version of wine. Does it work for you, or did you use Windows for
> the installation?

I used WINE (more precisely CrossOver Office 3.0.1 and CXO nightly) to 
install the program, which worked flawlessly.
Using the CXO config file, it also installed with the WINE CVS version.

Tobias