Re: shell32: FOF_MULTIDESTFILES must be set when copying files into directory
Thank you for your answer.
I agree, that before sending a patch I should write a test first. But the problem is that even a tests are ignored without any explanation. Maybe my tests aren't good, but why anybody just tell what's wrong.
Seems to be common habit on this list. I hope you're not added to the "Julliard ranking s**t list? Yes: REJECT" ( see http://www.winehq.org/?issue=353#WineConf%202008%20Keynote ) like i am. Spent loads of time for more than 3 years trying to help triaging bugs and helping out in appdb, only to find out that all my patches are ignored silently, but everybody seems to be happy with this policy, so i'll stop complaining. Personally this makes me quite sad. Hope you have more luck with your patches.
Have you tried asking on the #winehackers IRC channel? No, I doesn't use IRC. It's very hard to me to write in english in real-time
If people spent quite an amount of time trying to get a patch written, it shouldn't be too much trouble to just drop a note on wine-devel, what the reason is the patch got rejected. But i guess this takes more time then "the few seconds to see that the code is correct? If today is a busy day: IGNORE" Great feedback...
Ok, I try to resend my old tests, and write tests to other patches. Also there are some functions and stubs implemented, wchich doesn't require (as I think) a tests.
Send instant messages to your online friends http://uk.messenger.yahoo.com Send instant messages to your online friends http://uk.messenger.yahoo.com
On Sat, Oct 18, 2008 at 5:18 PM, Louis. Lenders <xerox_xerox2000(a)yahoo.co.uk> wrote:
Thank you for your answer.
I agree, that before sending a patch I should write a test first. But the problem is that even a tests are ignored without any explanation. Maybe my tests aren't good, but why anybody just tell what's wrong.
Vitaly claims he gets no feedback for his patches. He knows full well that I give him plenty of feedback. I explained to him what was wrong with his test SHFileOperation patches, yet he repeatedly sent the same patch to the list. I'm not going to repeat the same comment over and over again.
Seems to be common habit on this list. I hope you're not added to the "Julliard ranking s**t list? Yes: REJECT" ( see http://www.winehq.org/?issue=353#WineConf%202008%20Keynote )
Bologna. There is no such list. Every developer on the project has an unspoken trust rating, and every long term developer knows said rating for every other developer, especially Julliard. When you initially start submitting patches, your trust rating is low, understandably since we have no idea of your skill level. As you submit correct patches, your trust rating rises. When you repeatedly send bad patches, your rating drops. It is this trust rating that makes patch reviewers look at a patch from a guy with a low trust rating to say "Oh, another patch from that guy...it's probably not right." Our development model, with such a high bar for entry, is why the Wine project is such a clean, successful project. You can keep bitching about the way it works, or you can do what we all did in the beginning and get over that hump. Start small. Write exhaustive, well-written test cases for many features. Test cases are one of the easiest ways to get new code into the tree. After a while, start submitting tiny fixes that fix broken tests. Through this entire process, you will become a better developer, guaranteed. -- James Hawkins
Am Samstag, den 18.10.2008, 22:18 +0000 schrieb Louis. Lenders:
Spent loads of time for more than 3 years trying to help triaging bugs and helping out in appdb, only to find out that all my patches are ignored silently
Sorry, what do you mean by *all* of your patches? This is from the wine git log: 2008-09-25 Louis Lenders msi: Add stub for MsiSetExternalUIRecord. 2008-09-25 Louis Lenders shdocvw: Create default App Paths key for iexplore... 2008-09-04 Louis Lenders wine.inf: Add default Directx registry key for InstalledVersion. 2008-08-28 Louis Lenders shobjidl.idl: Add Taskbarlist interface definitions. 2008-08-28 Louis Lenders shlwapi: Fix UrlUnEscape to expand URLs in-place even... 2008-08-28 Louis Lenders shlwapi: Add test showing UrlUnEscape should convert... 2008-06-23 Louis Lenders d3dx9_*: Add version resources. 2008-06-21 Louis Lenders advapi32: Add stub for GetAuditedPermissionsFromAcl... 2008-06-20 Louis Lenders kernel32: Fix typo in SetProcessAffinityMask. 2008-06-10 Louis Lenders mscoree: Add stub for CorBindToCurrentRuntime. 2008-05-29 Louis Lenders wine.inf: Add fake glu32. 2008-04-18 Louis Lenders wininet: Improve stub for FindNextUrlCacheEntryW a... 2008-04-17 Louis Lenders urlmon: Add stub for CoInternetSetFeatureEnabled. 2008-03-12 Louis Lenders oleacc: Add GetOleaccVersionInfo. 2008-03-07 Louis Lenders shdocvw: Return something more useful for WebBrowser_get_Rea... 2008-02-29 Louis Lenders programs: Add a stubbed out secedit.exe. 2008-02-21 Louis Lenders shdocvw: Pretend success in WebBrowser_get_RegisterAsDropTarget. 2008-01-11 Louis Lenders shdocvw: Change return value for PersistMemory_Load. 2008-01-10 Louis Lenders user32: Add stub for GetLayeredWindowAttributes. [further patches back to 2005-12-06]
If people spent quite an amount of time trying to get a patch written, it shouldn't be too much trouble to just drop a note on wine-devel, what the reason is the patch got rejected.
In the case of the FOF_MULTIDESTFILES test patch that Vitaly didn't get in, I don't see an obvious problem in the last version, although that patch could be extended. I suspect (but I am definitely not an expert in this area), that the patch Vitaly Perov sent: | + /* move many files into directory with FOF_MULTIDESTFILES */ | + set_curr_dir_path(from, "test?.txt\0"); | + set_curr_dir_path(to, "testdir2\0"); | + retval = SHFileOperationA(&shfo2); | + todo_wine | + { | + ok(retval == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", retval); | + ok(file_exists("testdir2\\test2.txt"), "Expected the file 'test2.txt' to exist\n"); | + ok(file_exists("testdir2\\test4.txt"), "Expected the directory 'test4.txt' to exist\n"); | + } works because FOF_MULTIDESTFILES makes SHFileOperation pair from entries with to entries. As in this case, the from entry only has one component, namely "test?.txt" (even if it expands to multiple files), only one component of the destination is used regardless of FOF_MULTIDESTFILES. It would interesting to see what happens in the case that from contains two wildcard patterns, like "test?.txt\0test_?.txt\0" and dest contains two destinations like "testdir2\0testdir2\\nested\0". If my intuition is right, without FOF_MULTIDESTFILES everything is moved into testdir2, and with FOF_MULTIDESTFILES, test1.txt, test2.txt, test3.txt and the directory test4.txt is moved to testdir2, whereas test_5.txt is moved to testdir2\nested. But in the issue that started the thread I have to agree with James. FOF_MULTIDESTFILES is not to be set just because the destination is a directory. I quote Vitaly quoting MSDN: | FOF_MULTIDESTFILES | The pTo member specifies multiple destination files (one for each source | file in pFrom) rather than one directory where all source files are to be | deposited. In this sentence "rather than" is the same as "instead of". So FOF_MULTIDESTFILES indicates that the pTo member does *not* specify one destination directory, but a list of destination files instead. As the test that did not get committed seems to show (if it was tested in Windows, stating that helps getting a patch committed; sending a patch that does not work on any Windows version is risky, as it might put you on the infamous s**t list temporarily), the "multiple destination files" MSDN talks about might also be multiple destination directories.
Ok, I try to resend my old tests, and write tests to other patches. Also there are some functions and stubs implemented, wchich doesn't require (as I think) a tests. Even if you submit a stub, you usually do that because an application needs that function. In this case, it might be helpfull for people trying to implement that function to find an example use case in the testcase, so adding a todo_wine test is a nice thing, even if you just add a stub (it also shows in the test statistics that something needs to be done).
The tests do not only serve the purpose to test that Wine does what we expect wine to do (in fact, that is only a minor point of the tests), but more importantly, they should show that Wine and Windows do the *same* thing, so writing a testcase that passes after your patch has been applied to wine is a no-brainer unless you tested that this test also passes on Windows. If a patch that contains only a test did not get committed and you resubmit it, please add a note on which Windows version you tested it, if you didn't include it in your first try. General hint: If you can't test on windows for some reason (like you propose a direct3d test that needs a graphics card you don't own), do not send the testcase to wine-patches, but to wine-devel, and explain briefly why you can't test, and ask other developers to test it for you. This is usually not a problem. Regards, Michael Karcher
Vitaly claims he gets no feedback for his patches. He knows full well that I give him plenty of feedback. I explained to him what was wrong with his test SHFileOperation patches, yet he repeatedly sent the same patch to the list. I'm not going to repeat the same comment over and over again.
Yes, it was plenty of feedback from James. I was rewriting my test, James showed my errors, and send the test agan. But when I send the last version of my test (I thought it was good enough), I have received no reply. I resent it 2 or 3 times but no reply again.
In this sentence "rather than" is the same as "instead of". So FOF_MULTIDESTFILES indicates that the pTo member does *not* specify one destination directory, but a list of destination files instead
Yes, now I see my fault. Sometimes it's very difficult to translate such kind of expressions. I've translated this as "or"
I suspect (but I am definitely not an expert in this area), that the patch Vitaly Perov sent:
| + /* move many files into directory with FOF_MULTIDESTFILES */ | + set_curr_dir_path(from, "test?.txt\0"); | + set_curr_dir_path(to, "testdir2\0"); | + retval = SHFileOperationA(&shfo2); | + todo_wine | + { | + ok(retval == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", retval); | + ok(file_exists("testdir2\\test2.txt"), "Expected the file 'test2.txt' to exist\n"); | + ok(file_exists("testdir2\\test4.txt"), "Expected the directory 'test4.txt' to exist\n"); | + }
This test is not related to patch "shell32: FOF_MULTIDESTFILES must be set when copying files into directory" If the translation of "rather than" is "instead of", now I see my fault. But this test is passed in windows (win2k3). It doesn't pass in wine. So, it show difference between windows and wine behaviour. So, what's wrong in this test? -- Best wishes, Vitaly Perov Russia, Saint-Petersburg. www.etersoft.ru
Am Montag, den 20.10.2008, 19:04 +0400 schrieb Vitaly Perov:
I suspect (but I am definitely not an expert in this area), that the patch Vitaly Perov sent:
| + /* move many files into directory with FOF_MULTIDESTFILES */ | + set_curr_dir_path(from, "test?.txt\0"); | + set_curr_dir_path(to, "testdir2\0"); | + retval = SHFileOperationA(&shfo2); | + todo_wine | + { | + ok(retval == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", retval); | + ok(file_exists("testdir2\\test2.txt"), "Expected the file 'test2.txt' to exist\n"); | + ok(file_exists("testdir2\\test4.txt"), "Expected the directory 'test4.txt' to exist\n"); | + }
This test is not related to patch "shell32: FOF_MULTIDESTFILES must be set when copying files into directory" If the translation of "rather than" is "instead of", now I see my fault. But this test is passed in windows (win2k3). It doesn't pass in wine. So, it show difference between windows and wine behaviour. So, what's wrong in this test?
I honestly don't know. I can add that this test passes on XP SP3 too. It can be helpfull to include a history into the patch (saying: third resend: Changed <this and that>). One really has to ask Alexandre, or ask on wine-devel for feedback. Something like "Obviously, patch <http link here> did not get applied even after resending. Does anybody see anything obviously wrong with it?". As a guide to non-native speakers on #winehackers: Just ask (when julliard is online and not marked as ways) "julliard: What is wrong with <http link here>". Copy'n'paste all responses (recognizable by being from Alexandre or starting with your nickname) and try to make sense of them later. Regards, Michael Karcher
participants (4)
-
James Hawkins -
Louis. Lenders -
Michael Karcher -
Vitaly Perov