Re: [PATCH 3/6] COMDLG32: printdlg: check hook pointer (resend 3)

[Dmitry Timoshkov]

"Vitaly Lipatov" <lav(a)etersoft.ru> wrote:
> --- dlls/commdlg/printdlg.c 11 Jan 2006 11:37:03 -0000 1.100
> +++ dlls/comdlg32/printdlg.c 24 Feb 2006 17:47:53 -0000

The way you've generated this patch looks pretty strange.
> /* Call user paint hook if enable */
>      if (pda->dlga->Flags & PSD_ENABLEPAGEPAINTHOOK)
> -        if (pda->dlga->lpfnPagePaintHook(hwndDlg, uMsg, wParam, lParam))
> +        if (pda->dlga->lpfnPagePaintHook && pda->dlga->lpfnPagePaintHook(hwndDlg, uMsg, wParam, lParam))

You have to add the checks existing in PageSetupDlgA to PageSetupDlgW instead.

Also, in order to increase the probability of your patches to be included
should either point to existing bug that each of your patches fix, or/and
add a test case which doesn't pass without your patch and does with it.

-- 
Dmitry.

[PATCH 3/6] COMDLG32: printdlg: check hook pointer (resend 3)

[Vitaly Lipatov]

В сообщении от 9 декабря 2007 Dmitry Timoshkov написал(a):
> "Vitaly Lipatov" <lav(a)etersoft.ru> wrote:
> > --- dlls/commdlg/printdlg.c 11 Jan 2006 11:37:03 -0000 1.100
> > +++ dlls/comdlg32/printdlg.c 24 Feb 2006 17:47:53 -0000
>
> The way you've generated this patch looks pretty strange.
> Just a too old patch, partially updated by script.
> > /* Call user paint hook if enable */
> >      if (pda->dlga->Flags & PSD_ENABLEPAGEPAINTHOOK)
> > -        if (pda->dlga->lpfnPagePaintHook(hwndDlg, uMsg, wParam, lParam))
> > +        if (pda->dlga->lpfnPagePaintHook &&
> > pda->dlga->lpfnPagePaintHook(hwndDlg, uMsg, wParam, lParam))
>
> You have to add the checks existing in PageSetupDlgA to PageSetupDlgW
> instead.

Instead or also?
I guess, this check is needed in this place in any way.
> Also, in order to increase the probability of your patches to be included
> should either point to existing bug that each of your patches fix, or/and
> add a test case which doesn't pass without your patch and does with it.
> I do not know  a way to check print dialogs without installed printer drivers 
> in the system/wine.

-- 
Vitaly Lipatov, ALT Linux Team
Russia, Saint-Petersburg, www.etersoft.ru

[PATCH 3/6] COMDLG32: printdlg: check hook pointer (resend 3)

[Dmitry Timoshkov]

"Vitaly Lipatov" <lav(a)etersoft.ru> wrote:
> > > --- dlls/commdlg/printdlg.c 11 Jan 2006 11:37:03 -0000 1.100
> > > +++ dlls/comdlg32/printdlg.c 24 Feb 2006 17:47:53 -0000
> >
> > The way you've generated this patch looks pretty strange.
> > Just a too old patch, partially updated by script.

Looking at the file dates that doesn't sound plausibly. The patches
are supposed not just to be applied, but also tested with current
git sources.
> > > /* Call user paint hook if enable */
> > >      if (pda->dlga->Flags & PSD_ENABLEPAGEPAINTHOOK)
> > > -        if (pda->dlga->lpfnPagePaintHook(hwndDlg, uMsg, wParam, lParam))
> > > +        if (pda->dlga->lpfnPagePaintHook &&
> > > pda->dlga->lpfnPagePaintHook(hwndDlg, uMsg, wParam, lParam))
> >
> > You have to add the checks existing in PageSetupDlgA to PageSetupDlgW
> > instead.
> > Instead or also?
> > I guess, this check is needed in this place in any way.

No.
> > Also, in order to increase the probability of your patches to be included
> > should either point to existing bug that each of your patches fix, or/and
> > add a test case which doesn't pass without your patch and does with it.
> > I do not know  a way to check print dialogs without installed printer drivers 
> > in the system/wine.

That's a relatively rare case which is simply to avoid. Have a look at
the existing tests how they test printing functionality. In any case
having a bug report or posting a backtrace (you are trying to fix a crash,
right?) would help.

-- 
Dmitry.