Re: oledlg: Implement OleUIAddVerbMenuA/W functions [resend]

[Vitaly Lipatov]

В сообщении от 24 декабря 2008 Вы написали:
> On Wed, Dec 24, 2008 at 5:49 AM, Vitaly Lipatov  wrote:
> > Hello,
> >
> > Can anyone comment what bad with my
> > OleUIAddVerbMenuA/W implementation.
> >
> > I know no one is interested in this function at your side, but it is
> > really used function for more than one year.
> >
> > В сообщении от 20 декабря 2008 Vitaly Lipatov написал(a):
> > > Real implementation of the OleUIAddVerbMenu function.
> >
> > --
> > Vitaly Lipatov, ALT Linux Team
> > Russia, Saint-Petersburg, www.etersoft.ru
>
> Can you add a testcase for these functions?

I know it is common question about testcase, but I think it does not means 
with this functions.
Note, I do not fix some functions, I have implemented from scratch it instead 
stub.
And my better test case is a popular account application used this functions.

oledlg: Implement OleUIAddVerbMenuA/W functions [resend]

[Austin English]

On Wed, Dec 24, 2008 at 7:36 AM, Vitaly Lipatov  wrote:
> В сообщении от 24 декабря 2008 Вы написали:
> > On Wed, Dec 24, 2008 at 5:49 AM, Vitaly Lipatov  wrote:
> > > Hello,
> > >
> > > Can anyone comment what bad with my
> > > OleUIAddVerbMenuA/W implementation.
> > >
> > > I know no one is interested in this function at your side, but it is
> > > really used function for more than one year.
> > >
> > > В сообщении от 20 декабря 2008 Vitaly Lipatov написал(a):
> > > > Real implementation of the OleUIAddVerbMenu function.
> > >
> > > --
> > > Vitaly Lipatov, ALT Linux Team
> > > Russia, Saint-Petersburg, www.etersoft.ru
> >
> > Can you add a testcase for these functions?
>
> I know it is common question about testcase, but I think it does not means
> with this functions.
> Note, I do not fix some functions, I have implemented from scratch it instead
> stub.
> And my better test case is a popular account application used this functions.
>
> --
> Vitaly Lipatov, ALT Linux Team
> Russia, Saint-Petersburg, www.etersoft.ru

I'm not saying your patch is wrong by any means. Testcases serve as
documentation of the Win32 API, and also help prevent regressions in
those functions.

oledlg: Implement OleUIAddVerbMenuA/W functions [resend]

[Vitaly Lipatov]

В сообщении от 24 декабря 2008 Austin English написал(a):
> On Wed, Dec 24, 2008 at 7:36 AM, Vitaly Lipatov  wrote:

...
> > I know it is common question about testcase, but I think it does not
> > means with this functions.
> > Note, I do not fix some functions, I have implemented from scratch it
> > instead stub.
> > And my better test case is a popular account application used this
> > functions.

...
> I'm not saying your patch is wrong by any means. Testcases serve as
> documentation of the Win32 API, and also help prevent regressions in
> those functions.

I agree, but I have not idea how to make test for it. I wrote this code about 
one year ago and do not remember how it works :) But I use these functions 
every day in an accounting application.

oledlg: Implement OleUIAddVerbMenuA/W functions [resend]

[Nikolay Sivov]

Vitaly Lipatov wrote:
> В сообщении от 24 декабря 2008 Austin English написал(a):
> > On Wed, Dec 24, 2008 at 7:36 AM, Vitaly Lipatov  wrote:
>
> ...
> > > I know it is common question about testcase, but I think it does not
> > > means with this functions.
> > > Note, I do not fix some functions, I have implemented from scratch it
> > > instead stub.
> > > And my better test case is a popular account application used this
> > > functions.
>
> ...
> > I'm not saying your patch is wrong by any means. Testcases serve as
> > documentation of the Win32 API, and also help prevent regressions in
> > those functions.
>
> I agree, but I have not idea how to make test for it. I wrote this code about 
> one year ago and do not remember how it works :) But I use these functions 
> every day in an accounting application.

You could start with testing for some special cases (such as NULL input 
parameters).
For me your comment looks suspicious:
    -----

+ * TODO
    + *  Check if OLE object has no verbs

-----

+    hr = IOleObject_EnumVerbs(lpOleObj, &pEnumVerbs);
    +    if (hr == S_OK) {
  ----
Will this hr be OLEOBJ_E_NOVERBS when no verbs present?
If it's so you're actually checking for this.
This one needs a testcase for example.

At the end you return HMENU without NULL check
  ----

+    *lphMenu = popup;
    +    return TRUE;
    }
  -----

Does native crash on it or not? I'm not sure.

The same about SetLastError on failure - this should be checked by testing it directly.

oledlg: Implement OleUIAddVerbMenuA/W functions [resend]

[Vitaly Lipatov]

В сообщении от 25 декабря 2008 Вы написали:
...
> You could start with testing for some special cases (such as NULL input
> parameters).

If I will add test case for NULL input and returned values, my commit will be 
accepted?
> For me your comment looks suspicious:
> -
> - TODO
> -
> - Check if OLE object has no verbs
> -----
>
> - hr = IOleObject_EnumVerbs(lpOleObj, &pEnumVerbs);
> - if (hr == S_OK) {
>
> Will this hr be OLEOBJ_E_NOVERBS when no verbs present?
> If it's so you're actually checking for this.
> This one needs a testcase for example.

Please understand me. I did a part of work. Possible my implementation is 
incomplete. But I wish to share result of my work with the world, winehq 
community. I agree with test case argument with I fix already existing 
function. But I do a new realization and have no enough time and knowledge
to write ideal code. Also I can improve code only from winehq repository.
...
> Does native crash on it or not? I'm not sure.
>
> The same about SetLastError on failure - this should be checked by testing
> it directly.

I will try to add this tests.

oledlg: Implement OleUIAddVerbMenuA/W functions [resend]

[Nikolay Sivov]

Vitaly Lipatov wrote:
> В сообщении от 25 декабря 2008 Вы написали:
> ...
> > You could start with testing for some special cases (such as NULL input
> > parameters).
>
> If I will add test case for NULL input and returned values, my commit will be 
> accepted?

I don't know actually. That's first I think about when my patch isn't 
accepted.
> > For me your comment looks suspicious:
> > -
> > - TODO
> > -
> > - Check if OLE object has no verbs
> > -----
> >
> > - hr = IOleObject_EnumVerbs(lpOleObj, &pEnumVerbs);
> > - if (hr == S_OK) {
> >
> > Will this hr be OLEOBJ_E_NOVERBS when no verbs present?
> > If it's so you're actually checking for this.
> > This one needs a testcase for example.
>
> Please understand me. I did a part of work. Possible my implementation is 
> incomplete. But I wish to share result of my work with the world, winehq 
> community. I agree with test case argument with I fix already existing 
> function. But I do a new realization and have no enough time and knowledge
> to write ideal code. Also I can improve code only from winehq repository.
> ...

All of this is perfectly clear, and I don't argue with you and I'm not 
calling your patch bad.
I'm only point on some obvious things for me. Treat this only like an 
advice and my opinion.
> > Does native crash on it or not? I'm not sure.
> >
> > The same about SetLastError on failure - this should be checked by testing
> > it directly.
>
> I will try to add this tests.