Re: winmm: MCI system commands are not eligible for auto-open. (try 2)
<Joerg-Cyril.Hoehle(a)t-systems.com> 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. -- 1.5.6.3 [text/plain (2B)] -- Dmitry.
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.
<Joerg-Cyril.Hoehle(a)t-systems.com> 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. -- Dmitry.
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.
<Joerg-Cyril.Hoehle(a)t-systems.com> 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. -- Dmitry.
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.
<Joerg-Cyril.Hoehle(a)t-systems.com> 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. -- Dmitry.
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
<Joerg-Cyril.Hoehle(a)t-systems.com> 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. -- Alexandre Julliard julliard(a)winehq.org
participants (4)
-
Alexandre Julliard -
Dmitry Timoshkov -
Joerg-Cyril.Hoehle＠t-systems.com -
Juan Lang