On 06/09/11 10:55, Vincent Povirk wrote:
- if (pnButton) *pnButton = IDYES;
- if (pnRadioButton) *pnRadioButton = pTaskConfig->nDefaultButton;
- if (pfVerificationFlagChecked) *pfVerificationFlagChecked = TRUE;
- return S_OK;
I don't think it's appropriate to make selections for the user like this. If you can't present all of the choices to the user, you should probably return E_NOTIMPL.
Yes, I agree, if I had made that stub myself I would have returned E_NOTIMPL, but I merely moved it as-is from commctrl.c, I assume it was put there that way quickly to make some application work, I did not want to break anything.
Ah, you're right, I didn't notice that.
Thing is, as I mentioned earlier, the code is already all written and working. (You can download the DLL from my website and see for yourself)
Yeah, splitting an existing implementation can be tough.
On the other hand, the various features (progress bars, radio buttons, that dialog has a lot of stuff going on..) should be written so that they're independent from the other features and you can split them out in a sensible way (though it's still a hassle to actually do it).
I guess many of the elements could be shown or hidden, and each one affects the dialog size and the placement of everything else. But if you do this cleanly, it should still be easy to add each element.
I was hoping to read your existing code and see if anything struck me, but it seems you haven't provided it.
I would like some guidance on what constitutes an adequate patch size then. According to this, I shoud make a commit into my local git every time I do something simple. I can (and will) split this patch as you mention, but I fail to see how I could split my patches as I get into the meat of my implementation (as I mentioned earlier). Input in this area would be appreciated.
As I see it, the following guidelines apply: * The code must compile without warnings and pass all the tests after each patch is applied. Any non-passing tests must be marked as todo_wine. (As a consequence of this, if you fix Wine so that a failing test passes, you must remove the todo_wine as part of that same commit.) * No patch should introduce a memory leak, regression, or other bad thing. If you absolutely cannot avoid doing this (I think it's happened to me once), make a note that you've done this so that your badness doesn't get accepted without the later fix. * Try to avoid introducing unused code, resources, etc. That means adding internal functions as you begin to use them. (However, things like dialog or menu resources can be added as a whole, even if individual controls/menus are unused/unimplemented.) I've had to break this rule once or twice as well. * Each patch should contain a single logical change. * If you can split your patch into multiple logical changes without breaking anything in between, you probably should. * In some cases, you can get away with having two logical changes in a patch, if one of the changes is sufficiently small and related to the other. An example of a small change might be adding a function that only forwards to another that you implement (like TaskDialog/TaskDialogIndirect), or adding a small test that you also fix in Wine. But it's always OK to send multiple patches instead.
There's not really a guideline in terms of lines of code. It's more about how much logic you have in each patch, and how much it relates to the other logic. If it takes a long time for a reader (who we assume is already familiar with the code you're modifying) to understand all the logic you've introduced and how it all relates to each other, that's a sign that your patch is too big.
I would like to point out that your first patch was marked "Pending", not "Needs Splitting" (and also not "Needs tests"). That means Alexandre didn't necessarily object to the number of different things you changed in the patch, but he was uncomfortable with something about one of those changes, and now we don't know which it was. Anything you hear from anyone other than Alexandre is, of course, speculation. (I see "Pending" as meaning "This makes me nervous" while "Rejected" means "I know for sure this is wrong".)
Also how do I write a test that tests the ordinals? By checking that GetProcAddress(hMod, "TaskDialogIndirect") == GetProcAddress(hMod, "#345") ?
No, that would be silly. The most you should need to do there is explain in more detail why you think the ordinal is important and unlikely to change. But it seems to me you already explained that well enough (though I personally wouldn't know how to verify it), and so I really don't think the ordinals are the sticking point. (Come to think of it, there's a way to specify in the .spec file that programs should link by ordinal, and if MS does it then we probably should too. I think it's -ordinal.)
Now that I understand you're not making the existing stub worse, and given that your patch doesn't do very much, the "Pending" status is baffling to me. It makes me think I missed something obvious.
Maybe the problem is that your TaskDialog function doesn't set dwFlags? That would likely result in other invalid fields being used when they shouldn't. From MSDN, it appears many of the other fields should be initialized as well. (But that should have resulted in a "Rejected" status, so maybe he didn't notice that. Hmm.)
What I seem to get from your input and reading SubmittingPatches, is basically "tests, tests, tests". I thought a while about "how does one automate tests on GUI stuff?" and "how can I ensure that my dialog renders right in a test, short of using a flaky thing like GetPixel()?", etc.
Then it came to me. What I will do is first change the implementation to return E_NOTIMPL as Vincent Povirk suggested. Then I will start writing tests that show use TaskDialogIndirect and sends a bunch of TDM_xxx messages and tests that the callback procedures receives all the right TDN_xxx notifications, at least. It won't be able to test that the dialog _displays_ right, but at least that it functions right from an API point of view. As long as I can close the dialog when done testing, the tests should not require any kind of user interaction.
Well, I think you may be right that changing it to E_NOTIMPL at this point would break something and might be a bad idea. Sadly, Hans didn't say what the stub was originally for. Hopefully, we'll soon have a working implementation and it won't matter. ;)
I definitely think testing the notifications is the right idea, and making sure the display and interaction are correct is not as worthwhile. It's still possible to test display and interaction by accessing individual controls by id and using messages to interact with them. But I don't think it's worth doing that unless real programs are known to do that (they often do this with other common dialogs). I also don't know to what extent it's worth making your dialog template compatible with windows, or how to avoid potential copyright infringement when doing that. I hope someone else can chime in on this issue.
I think we may overemphasize the importance of tests. They are not always necessary for getting code into Wine, as our current spotty test coverage proves. Still, they are really nice to have.