February 2008 - Wine-Devel - list.winehq.org

Re: comctl32: Fix invalid syntax.

by Alexandre Julliard

"Bang Jun-young" <junyoung(a)mogua.com> writes: > 18 hours passed, and it looks like Alexandre decided to ignore this...why? > > This fix is required for Wine to be built with VC2005/2008 (although > Alexandre doesn't seem to care about it). There's no need to count the hours, or to ask every day, especially not with a mail to wine-patches. I have lots of pending patches from my vacation, I'll get to it eventually, and if I don't you should resubmit it after a week has passed. And if you want me to care about VC2005, maybe you should mention in your patch that this is the reason for your change, "fix invalid syntax" doesn't tell me anything. -- Alexandre Julliard julliard(a)winehq.org

Feb. 7, 2008

Re: comctl32: Fix invalid syntax.

by Bang Jun-young

18 hours passed, and it looks like Alexandre decided to ignore this...why? This fix is required for Wine to be built with VC2005/2008 (although Alexandre doesn't seem to care about it). On 2/6/08, Bang Jun-young <junyoung(a)mogua.com> wrote: > Any reason why this hasn't been committed? >

Feb. 7, 2008

Re: systray[2/4]: correctly handle icon addition/deletion

by James Hawkins

On Feb 6, 2008 6:24 PM, Kirill K. Smirnov <lich(a)math.spbu.ru> wrote: > В сообщении от Thursday 07 February 2008 02:58:42 вы написали: > > > Kirill K. Smirnov wrote: > > > @@ -71,6 +71,12 @@ struct icon > > > static struct tray tray; > > > static BOOL hide_systray; > > > > > > +static BOOL add_icon(NOTIFYICONDATAW *nid); > > > +static BOOL modify_icon(NOTIFYICONDATAW *nid); > > > +static BOOL delete_icon(const NOTIFYICONDATAW *nid); > > > +static BOOL delete_icon_directly(struct icon *icon); > > > +static BOOL display_icon(struct icon *icon, BOOL hide); > > > + > > > /* adaptor code */ > > > > > > #define ICON_SIZE GetSystemMetrics(SM_CXSMICON) > > > > I don't see any reason why you need to add forward declarations for all > > of these functions. > > Hmmmm..... It may be an artifact from my development when I need to call them > from places before they defined.... > Anyway, is it really bad to have them declared here? If so I'll remove it. > Forward declarations usually can, and should be, avoided. Just rearrange the code. -- James Hawkins

Feb. 7, 2008

Re: systray[3/4]: Better validate icon owner

by Kirill K. Smirnov

> I don't get why you need this change. PostMessage should correctly > handle the case where icon->owner has been destroyed and adding a call > to IsWindow just introduces a race condition. Just a testcase: 1) run any wine app (to be sure that explorer is running), e.g winecfg 2) run taskmgr 3) select taskmgr process in tasklist and kill it. taskmgr window will dissapear but tray icon not. 4) move mouse cursor over taskmgr tray icon. Before this patch it will not dissapear, but should. The reason is that ERROR_INVALID_HANDLE is not the only value which indicates the absence of owner window (ERROR_INVALID_WINDOW_HANDLE too). I think that using robust and straightforward IsWindow() function is better than verify last error. Is it acceptable to check return value of PostMessage only and do not check last error? And what race condition introduces IsWindow() here? -- Kirill

Feb. 7, 2008

Re: systray[2/4]: correctly handle icon addition/deletion

by Kirill K. Smirnov

В сообщении от Thursday 07 February 2008 02:58:42 вы написали: > Kirill K. Smirnov wrote: > > @@ -71,6 +71,12 @@ struct icon > > static struct tray tray; > > static BOOL hide_systray; > > > > +static BOOL add_icon(NOTIFYICONDATAW *nid); > > +static BOOL modify_icon(NOTIFYICONDATAW *nid); > > +static BOOL delete_icon(const NOTIFYICONDATAW *nid); > > +static BOOL delete_icon_directly(struct icon *icon); > > +static BOOL display_icon(struct icon *icon, BOOL hide); > > + > > /* adaptor code */ > > > > #define ICON_SIZE GetSystemMetrics(SM_CXSMICON) > > I don't see any reason why you need to add forward declarations for all > of these functions. Hmmmm..... It may be an artifact from my development when I need to call them from places before they defined.... Anyway, is it really bad to have them declared here? If so I'll remove it. -- Kirill

Feb. 7, 2008

Re: systray[3/4]: Better validate icon owner

by Reece Dunn

On 06/02/2008, Robert Shearman <rob(a)codeweavers.com> wrote: > Kirill K. Smirnov wrote: > > @@ -131,15 +130,18 @@ static LRESULT WINAPI adaptor_wndproc(HW > > case WM_LBUTTONDBLCLK: > > case WM_RBUTTONDBLCLK: > > case WM_MBUTTONDBLCLK: > > - /* notify the owner hwnd of the message */ > > - WINE_TRACE("relaying 0x%x\n", msg); > > - ret = PostMessage(icon->owner, icon->callback_message, (WPARAM) icon->id, (LPARAM) msg); > > - if (!ret && (GetLastError() == ERROR_INVALID_HANDLE)) > > + if (!IsWindow(icon->owner)) > > { > > WINE_WARN("application window was destroyed without removing " > > "notification icon, removing automatically\n"); > > delete_icon_directly(icon); > > } > > + else > > + { > > + /* notify the owner hwnd of the message */ > > + WINE_TRACE("relaying 0x%x\n", msg); > > + PostMessage(icon->owner, icon->callback_message, (WPARAM) icon->id, (LPARAM) msg); > > + } > > break; > > > > case WM_NCDESTROY: > > I don't get why you need this change. PostMessage should correctly > handle the case where icon->owner has been destroyed and adding a call > to IsWindow just introduces a race condition. Is there a test case in the PostMessage tests to verify that behaviour? - Reece

Feb. 7, 2008

Re: systray[2/4]: correctly handle icon addition/deletion

by Robert Shearman

Kirill K. Smirnov wrote: > @@ -71,6 +71,12 @@ struct icon > static struct tray tray; > static BOOL hide_systray; > > +static BOOL add_icon(NOTIFYICONDATAW *nid); > +static BOOL modify_icon(NOTIFYICONDATAW *nid); > +static BOOL delete_icon(const NOTIFYICONDATAW *nid); > +static BOOL delete_icon_directly(struct icon *icon); > +static BOOL display_icon(struct icon *icon, BOOL hide); > + > /* adaptor code */ > > #define ICON_SIZE GetSystemMetrics(SM_CXSMICON) > I don't see any reason why you need to add forward declarations for all of these functions. -- Rob Shearman

Feb. 6, 2008

Re: systray[3/4]: Better validate icon owner

by Robert Shearman

Kirill K. Smirnov wrote: > @@ -131,15 +130,18 @@ static LRESULT WINAPI adaptor_wndproc(HW > case WM_LBUTTONDBLCLK: > case WM_RBUTTONDBLCLK: > case WM_MBUTTONDBLCLK: > - /* notify the owner hwnd of the message */ > - WINE_TRACE("relaying 0x%x\n", msg); > - ret = PostMessage(icon->owner, icon->callback_message, (WPARAM) icon->id, (LPARAM) msg); > - if (!ret && (GetLastError() == ERROR_INVALID_HANDLE)) > + if (!IsWindow(icon->owner)) > { > WINE_WARN("application window was destroyed without removing " > "notification icon, removing automatically\n"); > delete_icon_directly(icon); > } > + else > + { > + /* notify the owner hwnd of the message */ > + WINE_TRACE("relaying 0x%x\n", msg); > + PostMessage(icon->owner, icon->callback_message, (WPARAM) icon->id, (LPARAM) msg); > + } > break; > > case WM_NCDESTROY: I don't get why you need this change. PostMessage should correctly handle the case where icon->owner has been destroyed and adding a call to IsWindow just introduces a race condition. -- Rob Shearman

Feb. 6, 2008

Re: systray[1/4]: rewrite tooltips support

by Kirill K. Smirnov

> Please explain better what you are doing, "rewrite tooltips support" > doesn't say what is changing or why the change is needed (and "rewrite" > is an exaggeration, you are mostly just moving code around). The main issue this patch fixes is an "attempt to set/modify a tooltip to a nonexistent window". This may happen when application modifies tip text while icon is hidden (e.g. when adding initially disabled icon with tooltip). [verbose] The change I did: 1) Store tip text in the icon structure. 2) Move tooltip creation code to the separate function. 3) Immediately bind tooltip to icon window at creation. We can bind empty tooltip text with icon window and further just update text when needed. 4) set_tooltip function was renamed to update_tooltip_text and simplified. Since tooltip support code is relatively small, this was enough for me to exaggeratedly proclaim tooltip support be rewritten ;-) [/verbose] So this ChangeLog is better: Avoid modifying non-existent tooltip windows of non-existent icon windows when application updates tip of hidden icon. -- Kirill

Feb. 6, 2008

Re: systray[1/4]: rewrite tooltips support

by Alexandre Julliard

"Kirill K. Smirnov" <lich(a)math.spbu.ru> writes: > diff --git a/programs/explorer/systray.c b/programs/explorer/systray.c Please explain better what you are doing, "rewrite tooltips support" doesn't say what is changing or why the change is needed (and "rewrite" is an exaggeration, you are mostly just moving code around). -- Alexandre Julliard julliard(a)winehq.org

Feb. 6, 2008