hello guys. i have took the endeavour of making my first dialog patch, managed to do it this weekend, to implement SetupPromptForDisk, which i stubbed but had some problems on the stub. gave me some big headaches to make this work x)
so here is the patch, i've made the two versions of the dialog, but before submiting i'd like your comments on it. what is wrong in it? in which file should it be? i left it in stubs.c but i think misc.c might be a better place. is it testable? i once asked this on IRC and got a "no" for response, is it not testable? should i put some comments? code documentation (or lack there of) is a flaw of mine...
i'm hoping for your replies so that i can submit as soon as possible. i've made some tests for "manual" testing and i have tested in "A vampire Story", the game that needed the stub, and it all worked fine. yhey! it lacks LOTS of details though, i just implemented the juice of it and some simple details.
regards.
From just looking at the patch (no testing or verifying the behavior
on windows):
You should add comdlg32 to DELAYIMPORTS, not IMPORTS, since it will be rarely needed and setupapi is used a lot. DELAYIMPORTS is like IMPORTS, but comdlg32 won't be loaded until you call a function that it provides.
You should handle the A/W split by invoking the W version first and then writing the A version as a wrapper around that (converting arguments to unicode and any unicode results back to ansi). There's no need to duplicate so much code.
You're using some lstr* functions instead of str* functions. The l functions are like the normal functions except that they catch invalid page faults. Unless you do not trust the pointers you are using AND you know that windows doesn't crash when they are invalid, you probably want to use the normal str* functions.
I think it's safe to assume that if you use GetModuleHandle from within the module whose handle you ask for, it will succeed. In fact, I think you can simply store the module handle in a global variable when it's first loaded and use that global variable later on. (Someone correct me if I'm wrong.)
Standard practice is to write tests mostly on the A version so that they can run on Windows 9x. MSDN claims the function is not available on Windows 9x; I guess it doesn't matter if that's true.
You've, uh, done something weird with the whitespace around SPDRP_MAXIMUM_PROPERTY in setupapi.h. My text editor displays that line broken in the middle, but the line numbers indicate it's one line.
I think this is big and new enough to warrant a new source file, perhaps a dialogs.c. (are there other dialogs in setupapi that need to be implemented?)
I'm curious what happens if you tell if you're looking for a file on C: that you know exists. If it doesn't bring up a dialog, that might allow you to create a few tests that succeed.
it lacks LOTS of details though, i just implemented the juice of it and some simple details.
This is EXACTLY what you should do. In fact, you may want to split some of the current details out into later patches.
Vincent Povirk
2009/2/17 Vincent Povirk <madewokherd+8cd9@gmail.commadewokherd%2B8cd9@gmail.com
I'm curious what happens if you tell if you're looking for a file on C: that you know exists. If it doesn't bring up a dialog, that might allow you to create a few tests that succeed.
Vincent Povirk
i could do that, if i had implemented IDF_CHECKFIRST :p that's one of the
details missing :) so i gather only the pre dialog implementation is testable? i can make a test on invalid argument then, because that's about it i have right now before dialog.
Hello Ricardo,
Ricardo Filipe wrote:
i have took the endeavour of making my first dialog patch, managed to do it this weekend, to implement SetupPromptForDisk, which i stubbed but had some problems on the stub. gave me some big headaches to make this work x)
so here is the patch, i've made the two versions of the dialog, but before submiting i'd like your comments on it. what is wrong in it? in which file should it be? i left it in stubs.c but i
Ahem ... git apply /tmp/0001-setupapi-implement-SetupPromptForDiskA-W.txt /tmp/0001-setupapi-implement-SetupPromptForDiskA-W.txt:25: trailing whitespace. * /tmp/0001-setupapi-implement-SetupPromptForDiskA-W.txt:254: trailing whitespace. struct PromptDiskParamsA *params = /tmp/0001-setupapi-implement-SetupPromptForDiskA-W.txt:265: trailing whitespace. struct PromptDiskParamsA *params = /tmp/0001-setupapi-implement-SetupPromptForDiskA-W.txt:373: trailing whitespace. struct PromptDiskParamsW *params = /tmp/0001-setupapi-implement-SetupPromptForDiskA-W.txt:384: trailing whitespace. struct PromptDiskParamsW *params = warning: squelched 9 whitespace errors warning: 14 lines add whitespace errors.
think misc.c might be a better place. is it testable? i once asked this on IRC and got a "no" for response, is it not testable? should i put some comments? code documentation (or lack there of) is a flaw of mine...
i'm hoping for your replies so that i can submit as soon as possible. i've made some tests for "manual" testing and i have tested in "A vampire Story", the game that needed the stub, and it all worked fine. yhey! it lacks LOTS of details though, i just implemented the juice of it and some simple details.
bye michael
-
Michael Stefaniuc -
Ricardo Filipe -
Vincent Povirk