Bug 4491

Created attachment 3311 [details] [review]
Patch to implement the changes described above.

Works OK for me, except that the .XWinrc system memu customization fails, for 
me, see:-

http://cygwin.com/ml/cygwin-xfree/2005-09/msg00152.html
(minor fix #if 0 to 1 at 604a588 in winmultiwindowwindow.c)

Also I couldn't compile Xming.exe with the winwindow.h addition (OK if missed 
out)

Colin Harrison

Created attachment 3330 [details] [review]
Updated patch; fixes SetupSysMenu.

This fixes the SetupSysMenu problem mentioned by Colin. I had it under "#if 0"
because I had not fully tested that change.

Note: I also have not yet tested NET_WM_ICON icons under Win2K.

My winwindow.h problem (when cross-compiling for Xming.exe using mingw):-

...
In file included from winmultiwindowwm.c:68:
winwindow.h:142: error: syntax error before "winInitMultiWindowClass"
winwindow.h:142: warning: type defaults to `int' in declaration of 
`winInitMultiWindowClass'
winwindow.h:142: warning: data definition has no type or storage class
...

can be fixed by copying in these lines (from winclipboard.h) to winwindow.h

/* Fixups to prevent collisions between Windows and X headers */
#define ATOM                    DWORD

A compromise fix for winwindow.h and Xming:-

--- ./programs/Xserver/hw/xwin/save_winwindow.h 2005-09-20 17:54:58.000000000 
+0100
+++ ./programs/Xserver/hw/xwin/winwindow.h      2005-09-20 17:40:51.000000000 
+0100
@@ -137,6 +137,11 @@
 /*
  * winmultiwindowicons.c
  */
+/* Fixup to prevent a syntax error in Xming */
+#ifdef __CYGWIN__
+ATOM
+winInitMultiWindowClass(BOOL Reset);
+#endif

 void
 winUpdateIcon (Window id);

And I was told to always declare all functions in the headers and 
follow 'coding standards':)

So far I've only seen kde NetIcons of weird? sizes, and no others e.g.
Window 3440608: found 34 x 34 NetIcon
Window 3440608: found 18 x 18 NetIcon
Window 3440608: no 32 x 32 NetIcon

I'm working on an update to this patch that:
1) Destroys icons from deleted windows, in place of code that destroys
per-window Class icons.
2) The NET_WM_ICON code for ARGB icons uses a 'blank' mask bitmap, copied from a
MSDN example. The mask should actually be filled out correctly, at least to make
sure Win2K icons will work.

There seem to be a lot of fixes in this patch. Would it make sense to break it 
up into smaller patches, each on addressing a single issue? It would make it 
easier to test and commit the fixes that are complete vs one that are still 
being tested and enhanced.

That makes sense to me as well Stuart.

Joe - what's the status on these ??

I'll take this on as Joe isn't responding?

Sorry about the phenomenal bug spam, guys.  Adding xorg-team@ to the QA contact so bugs don't get lost in future.

Created attachment 19690 [details] [review]
Minimal patch to correct icon data alignment

Created a new patch containing just the fix for the defect described (extacted from the original patch)

Thanks Jon, looks good.  Colin?

Jon's patch is OK, but the rest of Joe's stuff is good as well? However Joe never completely finished off, but as it is, it has worked well for me at all colour quality depths. Alpha icons still needs tidying and good simple test cases. The latter has stopped me from coding this further. _NET_WM_ICON icons can come in 'odd' sizes (as in other than Windows common sizes of 16, 32, 48 pixels). These can go wrong at 16bit colour quality etc. if the DIB route is not taken (via Joe's code).

Created attachment 20378 [details]
My current 16x16 and 32x32 net icon testcases

This is how I test the conversions at the moment.

Joe got a z-position wrong in winInitDialog()...
....
+  /* Center dialog window in the screen. Not done for multi-monitor systems, where
+   * it is likely to end up split across the screens. In that case, it appears
+   * near the Tray icon.
+   */
+  if (GetSystemMetrics(SM_CMONITORS)>1) {
+    /* Still need to refresh the frame change. */
+    SetWindowPos (hwndDlg, HWND_TOPMOST, 0,0,0,0,
+               SWP_NOMOVE | SWP_NOSIZE | SWP_FRAMECHANGED);
+  } else {
....

The dialog should not be just HWND_TOP IMH0.

bugzilla-daemon@freedesktop.org wrote:
> http://bugs.freedesktop.org/show_bug.cgi?id=4491
> 
> 
> 
> 
> 
> --- Comment #13 from Colin Harrison <colin.harrison@virgin.net>  2008-11-14 02:06:27 PST ---
> Jon's patch is OK, but the rest of Joe's stuff is good as well? However Joe
> never completely finished off, but as it is, it has worked well for me at all
> colour quality depths. Alpha icons still needs tidying and good simple test
> cases. The latter has stopped me from coding this further. _NET_WM_ICON icons
> can come in 'odd' sizes (as in other than Windows common sizes of 16, 32, 48
> pixels). These can go wrong at 16bit colour quality etc. if the DIB route is
> not taken (via Joe's code).
> 
I did have some mistakes in the code. I never got around to really 
debugging it, because I did not get feedback as to whether is was 
working or not. There were some inconsistencies in MS documentation, and 
I wasn't sure about using newer Alpha features. I can look into this 
again, now that the code is actually getting used.

Joe Krahn

Hi Joe,

Cygwin/X now use your code, but without using the CreateDIBSection

http://cygwin-ports.svn.sourceforge.net/viewvc/cygwin-ports/ports/trunk/xorg/xorg-server/cygwin-net_wm_icon-support.patch?revision=4968
This patch does not work well at color depths other than 32 (for _NET_WM_ICON)


I still use your NetWMToWinIcon() code in Xming, but not at odd sizes (i.e. not 16, 32, 48 pixels) when not 32bpp
http://www.straightrunning.com/XmingCode/patch4/winmultiwindowicons.c.patch

Thanks
Colin

Created attachment 20540 [details]
Xming's latest convertors

Add an alternative _NET_WM_ICON to HICON converter for use when not using a desktop colour depth of 32. Unlike the original, that used a DIB section and let Windows handle alpha, this one creates a DDB image and mask and just uses alpha as a threshold (127) to set icon opacity or transparency.

Created attachment 20546 [details] [review]
Xming's latest convertors

Honest..I tested this one, this time :)

Merged the data-alignment fix part of the patch (which fixes the headline issue) as commit bf65523ab0b39774f07a7ae478ff3f5653fad469

Leaving this bug open to track the other good stuff in here (although per comment #7 please feel free to raise separate bugs for those parts)

Joe: I notice as part of your patch you change things from using one class per window to using a single class. Is this just cleanup or does this fix some problem as well?

The other (very minor) aesthetic issue I notice with the icon converter is that if the X icon is noticably non-square, the converted image is aligned to the top-left of the windows icon (which must be square, I think), rather than being centered.

As a PS to my last patch..

Atom caching caching does not survive server regeneration

so

static Atom _XA_NET_WM_ICON;
static int generation;
...
if (generation != serverGeneration) {
    generation = serverGeneration;
     _XA_NET_WM_ICON = MakeAtom("_NET_WM_ICON", 12, FALSE);
}

...

in winXIconToHICON() is better.

Merged some parts extracted from this patch:

commit 996357e905c1082479bb238110b93bc170b8cb84
Cygwin/X: Update icons directly, rather than modifying the window's class

commit a72865868f03b675f86990476fcee601822894b3
Cygwin/X: Consolidate dialog initialization in winInitDialog()

I think that leaves: (1) change to a single class for all client windows, and (2) convert icons from _NET_WM_ICON hints

Changes to use a single native window class applied as 17e67c407d130c325d3899c18d68b8eef6a88bea

Created attachment 28480 [details] [review]
Updated NET_WM_ICON patch

Okay, I've taken the current icon converter code from Xming, added a slight tweak so we don't try to generate icons with alpha on Windows 2000, as that is still allegedly supported by Cygwin 1.7.

Hopefully this is ok to commmit and we can close this bug at last :-)

(This patch is also at http://sourceware.org/bugzilla/show_bug.cgi?id=10455)

With a few minor changes I sent Jon (for his patch above) this should now be OK.
Just under 4 years to do this must a record!
I'm sure that more fun could be had with the icon conversion code....
perhaps an alpha blend, there maybe something in pixman to help with this. But let's put this one to bed now.

Thanks,
Colin

_NET_WM_ICON conversion code pushed as commit a400dbb38f93030d51afe806b4b20d5ef501c855.

Thanks!