Re: Another systray patch

[Alexandre Julliard]

Mike Hearn  writes:
> There was a crash in the previous patch, here is a fixed version.
> - Add a new wineshell process, and put system tray handling in there
> - Rewrite the shell32 systray handling to be out of process
> - Support the freedesktop.org XEMBED protocol

It would be nice to do the XEMBED stuff as a separate patch. Also I
was hoping you would get rid of WS_EX_TRAYWINDOW instead of adding
even more uses of it...
> - /* we can't intern this with the rest as it depends on the screen we are connecting to */
> - systray_buffer = HeapAlloc(GetProcessHeap(), 0, sizeof(char)*20);
> - sprintf(systray_buffer, "_NET_SYSTEM_TRAY_S%d", DefaultScreen(display));

The screen will be 0 in 99.9% of the cases, you should preallocate
that one.
> +void *xmalloc(unsigned int size)
> +{
> - void *p = HeapAlloc(GetProcessHeap(), 0, size);
> -
> - if (!p)
> - {
> -
>    fprintf(stderr, "wineshell: virtual memory exhausted\n");
> -
>    exit(1);
> - }
> -
> - ZeroMemory(p, size);

exit(1) is not acceptable here, you should handle allocation failures
properly. Also malloc doesn't clear the buffer so xmalloc shouldn't
either, otherwise it's very confusing.
> +struct request {
> - NOTIFYICONDATAA ansi;
> - NOTIFYICONDATAW wide;
> - BOOL            unicode;
> - BYTE           *copydata;

Inter-process data should always be sent in Unicode.

Another systray patch

[Mike Hearn]

On Thu, 2004-12-09 at 15:35 +0100, Alexandre Julliard wrote:
> It would be nice to do the XEMBED stuff as a separate patch. Also I
> was hoping you would get rid of WS_EX_TRAYWINDOW instead of adding
> even more uses of it...

XEMBED in a separate patch is doable I guess but it wouldn't be used
without the rest of the patch. It's only use at the moment is the
systray code.
> The screen will be 0 in 99.9% of the cases, you should preallocate
> that one.

OK, I'll fix that.
> exit(1) is not acceptable here, you should handle allocation failures
> properly. Also malloc doesn't clear the buffer so xmalloc shouldn't
> either, otherwise it's very confusing.

OK on the second point. For the first how should it be handled? The
protocol Windows uses doesn't seem to let you return a success/failure
value. Do we just drop the tray icon?
> > +struct request {
> > - NOTIFYICONDATAA ansi;
> > - NOTIFYICONDATAW wide;
> > - BOOL            unicode;
> > - BYTE           *copydata;
>
> Inter-process data should always be sent in Unicode.

OK. I'll convert on the client side, that should clean the code up
somewhat too.

Another systray patch

[Alexandre Julliard]

Mike Hearn  writes:
> OK on the second point. For the first how should it be handled? The
> protocol Windows uses doesn't seem to let you return a success/failure
> value. Do we just drop the tray icon?

Yeah, something like that. It's OK to not add an icon if we are out of
memory, but it's not OK for the whole process to die.