Re: [v3 03/12] comctl32: Added basic implementation for task dialogs and add tests
On 10.03.2017 19:21, Fabian Maurer wrote:
+ /* Skip this test on wine, because it doesn't really fail, + * it would displays a dialog that doesn't automatically close */ + if (strcmp(winetest_platform, "wine")) + { + ret = pTaskDialogIndirect(&info, NULL, NULL, NULL); + ok(ret == S_OK, "Expected S_OK, got %x\n", ret); + }
We usually use winetest_interactive for that. There might be more comments, so please wait a bit before resending. ;) Also, please try to send your changes in reasonable sized chunks (max 5-7) in the future. Sending more patches at once only makes sense if they do not depend on each other. Best regards, Sebastian
We usually use winetest_interactive for that. Is that necessary if I replace the test with another in the next patch anyways? I figured it would be enough if I don't introduce a test fail, in case of a bisect or something along the lines.
Also, please try to send your changes in reasonable sized chunks (max 5-7) in the future. Sending more patches at once only makes sense if they do not depend on each other. I don't quite understand what you mean. You mean I sent too many patches at once? But all those 12 patches depend on each other, together only they form an implementation that could replace the current one.
Regards, Fabian Maurer
On 10.03.2017 20:18, Fabian Maurer wrote:
We usually use winetest_interactive for that. Is that necessary if I replace the test with another in the next patch anyways?
Sorry, I missed that it is only a workaround for unimplemented functionality. It should be fine then. Nevertheless, it actually wouldn't hurt to add a couple of interactive tests, too. Having non-interactive tests is nice, but they don't help to notice visual bugs.
I figured it would be enough if I don't introduce a test fail, in case of a bisect or something along the lines.
Also, please try to send your changes in reasonable sized chunks (max 5-7) in the future. Sending more patches at once only makes sense if they do not depend on each other. I don't quite understand what you mean. You mean I sent too many patches at once? But all those 12 patches depend on each other, together only they form an implementation that could replace the current one.
Yes, you should send your patches in smaller chunks. Also, you shouldn't rely on the assumption that all of your patches are applied at once. The patchset should still make sense, even if each commit is applied separately. If it doesn't, that would mean you have to reorder or merge them. As long as you mark failing tests with todo_wine it is not a problem to have a temporary state with "less features" than the original implementation though. Best regards, Sebastian
Nevertheless, it actually wouldn't hurt to add a couple of interactive tests, too. Having non-interactive tests is nice, but they don't help to notice visual bugs. That's true, I just didn't know how to do that. I'll probably add a few once this implementation is finished.
Yes, you should send your patches in smaller chunks. Also, you shouldn't rely on the assumption that all of your patches are applied at once. The patchset should still make sense, even if each commit is applied separately. If it doesn't, that would mean you have to reorder or merge them. As long as you mark failing tests with todo_wine it is not a problem to have a temporary state with "less features" than the original implementation though. Sorry, I still don't understand what's wrong. The patches depend on each other, so just applying patch 9 would result in an error. But only applying 1-5 should pose no problem. Except for missing functionality of course, but the tests should be fine. But I have to send these 12 patches as single patchset, since it's supposed to replace the old taskdialog-hack and I don't want to introduce a regression. Only with all those patches the functionality is the same.
Regards, Fabian Maurer
On 10.03.2017 21:23, Fabian Maurer wrote:
Yes, you should send your patches in smaller chunks. Also, you shouldn't rely on the assumption that all of your patches are applied at once. The patchset should still make sense, even if each commit is applied separately. If it doesn't, that would mean you have to reorder or merge them. As long as you mark failing tests with todo_wine it is not a problem to have a temporary state with "less features" than the original implementation though.
Sorry, I still don't understand what's wrong. The patches depend on each other, so just applying patch 9 would result in an error. But only applying 1-5 should pose no problem. Except for missing functionality of course, but the tests should be fine. But I have to send these 12 patches as single patchset, since it's supposed to replace the old taskdialog-hack and I don't want to introduce a regression. Only with all those patches the functionality is the same.
Of course they should be applied in the correct order, but introducing a temporary regression because of unimplemented functionality is not a problem, and basically expected when you are working on such a task. ;) Again, the usual and recommended process for upstreaming stuff is to focus on a set of about 5-7 patches, improve them until they fulfill all the requirements and have been accepted, and then proceed with next ones. Following these rules will make things easier for you (no need to resend all 12 patches when you missed some testbot errors, for example), and also for reviewers. As a reviewer you cannot be sure that a patch which previously was fine hasn't been changed, so you have to start from scratch and review the same patch over and over again - noone wants to do that. ;) If it is still not clear what I mean, maybe someone else can try and explain it a bit better. Best regards, Sebastian
On 11.03.2017 0:10, Sebastian Lackner wrote:
On 10.03.2017 21:23, Fabian Maurer wrote:
Yes, you should send your patches in smaller chunks. Also, you shouldn't rely on the assumption that all of your patches are applied at once. The patchset should still make sense, even if each commit is applied separately. If it doesn't, that would mean you have to reorder or merge them. As long as you mark failing tests with todo_wine it is not a problem to have a temporary state with "less features" than the original implementation though.
Sorry, I still don't understand what's wrong. The patches depend on each other, so just applying patch 9 would result in an error. But only applying 1-5 should pose no problem. Except for missing functionality of course, but the tests should be fine. But I have to send these 12 patches as single patchset, since it's supposed to replace the old taskdialog-hack and I don't want to introduce a regression. Only with all those patches the functionality is the same.
Of course they should be applied in the correct order, but introducing a temporary regression because of unimplemented functionality is not a problem, and basically expected when you are working on such a task. ;)
I think it is a problem in this case, it's not guaranteed they will all committed at once. One way to fix that is to incrementally add things, still falling back to MessageBox if something is not working yet.
Again, the usual and recommended process for upstreaming stuff is to focus on a set of about 5-7 patches, improve them until they fulfill all the requirements and have been accepted, and then proceed with next ones. Following these rules will make things easier for you (no need to resend all 12 patches when you missed some testbot errors, for example), and also for reviewers. As a reviewer you cannot be sure that a patch which previously was fine hasn't been changed, so you have to start from scratch and review the same patch over and over again - noone wants to do that. ;) If it is still not clear what I mean, maybe someone else can try and explain it a bit better.
Yes, keeping is at 5 at a time is better.
Best regards, Sebastian
On Freitag, 10. März 2017 22:36:56 CET Nikolay Sivov wrote:
On 11.03.2017 0:10, Sebastian Lackner wrote:
On 10.03.2017 21:23, Fabian Maurer wrote:
Yes, you should send your patches in smaller chunks. Also, you shouldn't rely on the assumption that all of your patches are applied at once. The patchset should still make sense, even if each commit is applied separately. If it doesn't, that would mean you have to reorder or merge them. As long as you mark failing tests with todo_wine it is not a problem to have a temporary state with "less features" than the original implementation though.
Sorry, I still don't understand what's wrong. The patches depend on each other, so just applying patch 9 would result in an error. But only applying 1-5 should pose no problem. Except for missing functionality of course, but the tests should be fine. But I have to send these 12 patches as single patchset, since it's supposed to replace the old taskdialog-hack and I don't want to introduce a regression. Only with all those patches the functionality is the same.
Of course they should be applied in the correct order, but introducing a temporary regression because of unimplemented functionality is not a problem, and basically expected when you are working on such a task. ;)
I think it is a problem in this case, it's not guaranteed they will all committed at once. One way to fix that is to incrementally add things, still falling back to MessageBox if something is not working yet.
Again, the usual and recommended process for upstreaming stuff is to focus on a set of about 5-7 patches, improve them until they fulfill all the requirements and have been accepted, and then proceed with next ones. Following these rules will make things easier for you (no need to resend all 12 patches when you missed some testbot errors, for example), and also for reviewers. As a reviewer you cannot be sure that a patch which previously was fine hasn't been changed, so you have to start from scratch and review the same patch over and over again - noone wants to do that. ;) If it is still not clear what I mean, maybe someone else can try and explain it a bit better.
Yes, keeping is at 5 at a time is better.
I really don't see how this would work. Sure, I could add a partial implementation, but it would be pretty useless until the other patches are added. Should we add code that's unused yet? I don't have a problem with that, but I thought it's against the project's rules. Falling back to the messagebox if someone is working sounds pretty impossible. You arguably need text and buttons and them working properly for the taskdialog, or you don't need to bother using it. I could delay the the cancel functionality, setting the correct window size, the default button, the callback and all the tests, but honestly I don't think that would be a good idea. Or just make the patches bigger by merging them. But I don't see how I could get a working taskdialog in just 5 patches. So, how should I go about it? Regards, Fabian Maurer
participants (3)
-
Fabian Maurer -
Nikolay Sivov -
Sebastian Lackner