[v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

[Fabian Maurer]

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