Re: winmm: MCI system commands are not eligible for auto-open. (try 2)

[Dmitry Timoshkov]

 wrote:
> - FIXME("(%04x) vkey %04X stub\n", dwFlags, lpParms->nVirtKey);

That change is unwanted.
> - if (dwFlags & MCI_NOTIFY)
> - mciDriverNotify((HWND)lpParms->dwCallback, wDevID,
> -
>                    (dwRet == 0) ? MCI_NOTIFY_SUCCESSFUL : MCI_NOTIFY_FAILURE);
> -
> - if (MMSYSERR_NOERROR==dwRet && (dwFlags & MCI_NOTIFY))
> -
>    mciDriverNotify((HWND)lpParms->dwCallback, wDevID, MCI_NOTIFY_SUCCESSFUL);
> return dwRet;
>
> }
>
> @@ -1903,10 +1906,9 @@ static	DWORD MCI_Sound(UINT wDevID, DWORD dwFlags, LPMCI_SOUND_PARMSW lpParms)
>          dwRet = sndPlaySoundW(lpParms->lpstrSoundName, SND_SYNC) ? MMSYSERR_NOERROR : MMSYSERR_ERROR;
>      else
>          dwRet = MMSYSERR_ERROR; /* what should be done ??? */
> - if (dwFlags & MCI_NOTIFY)
> - mciDriverNotify((HWND)lpParms->dwCallback, wDevID,
> -
>                    (dwRet == 0) ? MCI_NOTIFY_SUCCESSFUL : MCI_NOTIFY_FAILURE);
>
> - if (MMSYSERR_NOERROR==dwRet && (dwFlags & MCI_NOTIFY))
> -
>    mciDriverNotify((HWND)lpParms->dwCallback, wDevID, MCI_NOTIFY_SUCCESSFUL);
> return dwRet;
>
> }

Do you have a test case which shows that notofication is not sent is the failure case?

Also 'if (MMSYSERR_NOERROR==dwRet' misses spaces, and having comparison reversed doesn't
match the style of the surrounding code.

winmm: MCI system commands are not eligible for auto-open. (try 2)

Dmitry Timoshkov wrote:
> > - FIXME("(%04x) vkey %04X stub\n", dwFlags, lpParms->nVirtKey);
>
> That change is unwanted.

I can remove it, but why? Is supporting break keys a WONTFIX in Wine?
> Do you have a test case which shows that notofication is not sent is the failure case?

It's already in mci.c:
    test_notification(hwnd, "sound notify", err ? 0 : MCI_NOTIFY_SUCCESSFUL);
It's how all commands I've tested to some depth so far behave.
> having comparison reversed doesn't
> match the style of the surrounding code.

Ok.  After being bitten at least once by assignment/comparison
mismatch I promised myself to use that style.  I'm myself used
to read code as "if A equals 3" rather than "if 3 is the value
of A" but I'm convinced that's just a matter of getting used
to this style that is less error-prone in C.

Regards,
 Jörg Höhle.

winmm: MCI system commands are not eligible for auto-open. (try 2)

[Dmitry Timoshkov]

 wrote:
> > > - FIXME("(%04x) vkey %04X stub\n", dwFlags, lpParms->nVirtKey);
> >
> > That change is unwanted.
>
> I can remove it, but why? Is supporting break keys a WONTFIX in Wine?

If that's really a problem then the first thing to do is start with some bug
reports or test cases. Adding spurious FIXME's is not an improvement.
> > Do you have a test case which shows that notofication is not sent is the failure case?
>
> It's already in mci.c:
>     test_notification(hwnd, "sound notify", err ? 0 : MCI_NOTIFY_SUCCESSFUL);
> It's how all commands I've tested to some depth so far behave.

I don't see any correspnding changes in that patch that remove todo_wine statements
in the tests. That suggests that either such tests don't exist, or that change actually
fixes nothing.

winmm: MCI system commands are not eligible for auto-open. (try 2)

Dmitry Timoshkov wrote:
> I don't see any corresponding changes in that patch that remove todo_wine statements
> in the tests. That suggests that either such tests don't exist, or that change actually
> fixes nothing.

A third explanation is possible:
It's an example of one bug shadowing another bug.

There's no todo_wine to remove because
a) No Notification was sent in Wine because the parser did
   not correctly handle system commands.  MCI_Sound was never called so
   there was no notification.
b) There was no need for todo_wine in the past because this double error could
   not be detected.  No notification was and is the correct behaviour when the
   sound command returns an error, as it currently does.
b) When I fixed that in "try 1", AJ observed the notification test failure.
   That's why I had to fix the notification as well for "try 2".

The tests pass on MS (in both MMSYS_NOERROR and MCIERR_HARDWARE modes) proving they are ok.

Regards,
 Jörg Höhle.

winmm: MCI system commands are not eligible for auto-open. (try 2)

[Dmitry Timoshkov]

 wrote:
> > I don't see any corresponding changes in that patch that remove todo_wine statements
> > in the tests. That suggests that either such tests don't exist, or that change actually
> > fixes nothing.
>
> A third explanation is possible:
> It's an example of one bug shadowing another bug.
>
> There's no todo_wine to remove because
> a) No Notification was sent in Wine because the parser did
>    not correctly handle system commands.  MCI_Sound was never called so
>    there was no notification.
> b) There was no need for todo_wine in the past because this double error could
>    not be detected.  No notification was and is the correct behaviour when the
>    sound command returns an error, as it currently does.

Then that's what you need to fix in the first place.
> b) When I fixed that in "try 1", AJ observed the notification test failure.
>    That's why I had to fix the notification as well for "try 2".
>
> The tests pass on MS (in both MMSYS_NOERROR and MCIERR_HARDWARE modes) proving they are ok.

Since there is no neither todo_wine statements in the tests, nor test failures
under Wine that means that both the tests and the patch are not OK.

winmm: MCI system commands are not eligible for auto-open. (try 2)

Dmitry Timoshkov wrote:
> Since there is no neither todo_wine statements in the tests, nor test failures
> under Wine that means that both the tests and the patch are not OK.

What has Wine to say when talking about the correctness
of tests?  Only native dictates what the test result should be.
The emerging rule is: "no notification in case of error", and there's even a
specific test case about the MCI sound string command in the tests.

I believe you forgot that AJ mentioned a notification test failure that
appeared with "try 1": So there was a test failure under Wine.
> > b) There was no need for todo_wine in the past because this double error could
> >    not be detected.  No notification was and is the correct behaviour when the
> >    sound command returns an error, as it currently does.
>
> Then that's what you need to fix in the first place.

I cannot demonstrate the value of a fix to MCI_SOUND via a test case if the parser
never calls MCI_SOUND.  That's why the order in my git tree and that of submission is:
1. Get MCI_SOUND called (and fix the notification because of
   the test failure this introduces).
2. Get MCI_SOUND to actually play a sound (in my queue, past the 64bit patch).

I find it illogical to write a patch that would prefix an existing test
with todo_wine.  I aim at eliminating existing todo_wine.

Now I think the subject of my patch is misleading.  It should have read
"winmm: Correctly deal with MCI system commands." (to some extent).

Regards,
 Jörg Höhle.

winmm: MCI system commands are not eligible for auto-open. (try 2)

[Dmitry Timoshkov]

 wrote:
> > Since there is no neither todo_wine statements in the tests, nor test failures
> > under Wine that means that both the tests and the patch are not OK.
>
> What has Wine to say when talking about the correctness
> of tests?  Only native dictates what the test result should be.

Of course the tests are suppose to show the behaviour of Windows.
> The emerging rule is: "no notification in case of error", and there's even a
> specific test case about the MCI sound string command in the tests.

And another rule is that the patch which changes the behaviour of an API
needs to have an appropriate test case which does not pass before the patch
(i.e. has the todo_wine around it), and passes after the patch (i.e. the patch
removes the corresponding todo_wine). Your patch doesn't qualify for that.

winmm: MCI system commands are not eligible for auto-open. (try 2)

[Juan Lang]

Hi Dmitry,
> And another rule is that the patch which changes the behaviour of an API
> needs to have an appropriate test case which does not pass before the patch
> (i.e. has the todo_wine around it), and passes after the patch (i.e. the patch
> removes the corresponding todo_wine). Your patch doesn't qualify for that.

I think this is a good guideline, but I think Joerg justified his
approach reasonably well:  his first attempt introduced a test failure
because it caused a previously spuriously passing test to fail.  His
choices are to resubmit this patch, marking the test todo_wine along
with an explanation, and follow up with the patch that really fixes
the test, or to submit the two combined.  I think both are reasonable
if the explanation is clear enough.

The rule clearly applies when a test was not spuriously passing
before, e.g. is marked todo_wine, but in cases when the test is
spuriously passing, the "tests must always pass" rule also comes into
play.  The tests must always pass rule takes precedence, making the
tests should only ever remove a todo_wine rule impossible to meet.
--Juan

winmm: MCI system commands are not eligible for auto-open. (try 2)

[Alexandre Julliard]

 writes:
> Ok.  After being bitten at least once by assignment/comparison
> mismatch I promised myself to use that style.  I'm myself used
> to read code as "if A equals 3" rather than "if 3 is the value
> of A" but I'm convinced that's just a matter of getting used
> to this style that is less error-prone in C.

gcc prints a warning in most cases where you could make the mistake, so
it's not a good enough reason to make the code harder to read.