[PATCH] Bigger Chooser 2

Robert Collins rbcollins@cygwin.com
Sun Mar 30 13:09:00 GMT 2003


On Sun, 2003-03-30 at 18:20, Gary R. Van Sickle wrote:

> > So:
> > RECTPP.h would be the correct name.
> >
> > Having said that RECTPP is not any of the ok naming conventions for
> > classes.
> >
> > A quick recap:
> > The GNU standards are acceptable, even though I think their application
> > to C++ is less than effective.
> > Alternative, capitalised meaningful words. (my preference).
> >
> 
> Huh?  When did this restriction go into effect? 

From memory, when I started doing the non-gnu-standards class names :}.

>  Now, I'll admit it ain't the
> greatest name I ever came up with....
> 
> > i.e.
> > rect_pp (GNU)
> > Rectangle  (best)
> >
> 
> It's named "RECTPP" because it's a struct (not class) derived from Windows'
> RECT.  It's intended to be a memory-image-compatible lightweight "wrapper" of
> GDI's RECT (i.e., you can freely treat either one as the other one at any time
> however you got to it), and it seems to me not unreasonable for the name to make
> that as clear as possible.  It is not intended to be a "real object" as
> "Rectangle" or even rect_pp would imply.

Sure. I agree that the name should make that clear. I miss completely
how RECTPP makes that clear.

> > within the header, you mix interface and implementation. As I said to
> > Igor in the last week or so, please don't do this except for one-liners.
> > (And there while I will accept it, it's not preferred).
> >
> 
> Unfortunately I wrote it before last week.  I agree 99.44% with that philosophy.
> The 0.56% is number of methods related; if I had added any more methods, or
> exceeded a half page of code total, I'd likely have added a .cc.

I wasn't clear above - sorry. I meant that the in-declaration method
bodies should be moved to inlines in the header, not that a .cc was
needed.

> > void offset(int x, int y); is misnamed IMO. void move(int x, int y);
> > would be more intuitive to someone reading the code.
> >
> 
> That's MS's name for the functionality I'm duplicating:
> 
> BOOL OffsetRect(
>   LPRECT lprc,  // rectangle
>   int dx,       // horizontal offset
>   int dy        // vertical offset
> );

Ok. Be that as it may. Check out offset in a thesaurus. It does not mean
'move one thing by a vector'. It's closest synonym to that is to
compensate. 

Your offset method has a comment in it that says '    // Move the whole
rect by [x,y]'. This is a clear indication that the method is not self
explanatory. Naming it 'move' or even 'offsetBy' would make the method
fully selfexplanatory. move as a verb is the closest fit to what you are
doing in the method, which is why I suggested it was a better name.
offsetBy would be less clear, but if you really want a MS correlation to
that, then I can accept that.

> Again, the idea behind RECTPP is to wrap RECT, not to be a general-purpose
> Rectangle class.

In which case, you should document that in the header. I'm not going to
*guess* that you are aiming for memory layout compatability with the MS
RECT class if you don't write that down somewhere.

> > Lastly
> > CINSTALL is deprecated. We renamed setup from cinstall to setup when we
> > created the new cvs repository.
> >
> 
> Conceeded, but you are not seriously going to 'dock' me for the name of the
> *multiple-include-prevention define*, are you? ;-)

Dock you? No.
Require that it be changed before the patch is accepted? Yes.

> > Ok, onto the main patch.
> > I don't like the overloading of OnActivate. A query method would be more
> > appropriate.
> > i.e.
> > virtual bool toBeShown() const;
> >
> 
> People in Hell want icewater.  Maybe that would be better, who knows, that's not
> what I coded.

Well, what you coded, in that instance, is a step away from clean
design, not towards it. As a step towards clean design is available,
there is no need for us to commit that change. So: we won't.

>   What my OnActivate() changes do is allow the AntiVirus page (and
> future ones like it) to no longer need to know what the next page is if all they
> want to do is not be displayed. 

Yes.

>  That was the problem it was intended to solve.
> It wasn't intended to be the best possible way in history to do this.  

I'm not aiming for the 'best possible way'. I'm pointing out that rather
than changing the return value from 6? 7? OnActivate routines, you
simply need to add one virtual in the parent, override in the AntiVirus
class, and change the detection code to check with the query method
before calling OnActivate. Thats both easier to code and a smaller
patch.

> It was
> intended to get from A to B.  It does that.  Getting to C can be addressed at a
> later date.  In a smaller patch.  That does only that one change.

That one change will involve backing out 6-7 changes you are suggesting
in your current patch. Why should we take a step backwards to take one
forwards?

> > Why are you removing the __attribute__ ((noreturn)) in LogSingleton?
> >
> 
> Because it returns unless there's an error now:
> 
> * LogFile.cc (LogFile::exit): Add logic to only display a dialog box at
> 	exit if something went wrong.  Only exit() on error, otherwise return
> 	to caller.

Ah, must have missed part of the code - sorry.

> > I'm stopping here.
> 
> Then you didn't even look at the "main patch" Rob.  The "Bigger Chooser" stuff?
> 31k?

Well, it's commingled. You want me to look at the Bigger Chooser stuff?
Send just it in.

> So you decided to reject this patch and then went looking for justification?  I
> went to a lot of work to get this stuff working Rob.  I'd think the least you
> could do is actually look at the meat of the patch before rejecting it
> out-of-hand.

I rejected the patch on a heuristic that I have found very reliable. You
complained that I should review first, so I went and reviewed. After I
found that *some of* the patch was not acceptable, I bounced the entire
patch. 

I'm not about to review the entire thing - once I find more than a
couple of things that need changing, I'll document those clearly, and
suggest that the approach be propogated throughout if appropriate. Thats
YET ANOTHER side effect of commingled patches.

I didn't go "looking for justification". I know you went to a lot of
work to get the stuff working.

> > This won't go in as is.
> 
> Am I then left to *guess* what you're going to accept?

No. I have told you the requirements:
Split it up. One concept per patch. Size not the issue, commingled
orthogonal or splinter patches the issue.

> > It also won't go in as a uber-patch.
> 
> "uber" meaning what now?

super-patch. In this case a patch that does multiple things.

>   You again mention size below, is that the "uber" of
> which you speak?  I maintain again that a good 31k of this patch, i.e. the
> bigger chooser part, the majority of the patch, the reason we're here now, is
> simply not reducible.  Are you going to reject a 31k patch?  What's the size
> cutoff here?

If it's a 31K do-one-thing patch, I will accept it. Your's isn't.

> > Finally:
> > I don't recall a patch for the OnActivate change coming through.
> 
> I never submitted one and I hope I didn't indicate that I had.

No, you didn't indicate that - I was crossing my t's. 

> Huh?  The only such assumption I made was that you'd give me hell if my
> Changelog didn't have it's i's crossed and t's dotted.  I assumed I would have
> *HAD* to submit a big patch *regardless* if I wanted to submit *another* patch
> in the first-patch-submission-to-cvs-committal period.

Thats the point I'm making: you don't have to. You have to submit
separate patches regardless. 

> >And that
> > assumption you have made is a large contributor to the extra work you
> > are now being asked to make.
> 
> I'm not following at all now, sorry.
> 
> Well, tell you what.  I'll break out the icon and submit that as a separate
> patch; I have to assume that has little chance of being controversial. 

And it's been approved for checkin. 

>  As to
> the rest of this, you'll have to tell me how (or if) you'd like to proceed.
> Neither of us has the time for me to trial-and-error patches at you only to have
> them rejected because of a stray CINSTALL or an "I'd have done it different".

What part of my requests to date has been unclear? I'm at a loss as to
how it can be trial and error:

You've had a number of points addressed in my review.
Key reminder of the things I've requested that I still need done before
I can accept a patch:
* separate patches for separate things
* OnActivate is not a good place to change to do the 'does the window
want to be shown test'.
* rect.h changes

Basically Gary, I need separate patches. You've complained that my
review was not comprehensive enough, after claiming that commingled
patches are easier to review: Consider my review as proof positive that
commingled patches are not easier to review.

========
I'd like you proceed in the following manner:
* Generate a separate single-purpose patch for any pre-requisites to the
'big chooser patch'.
* Generate (a patch | patches) to be applied ontop of those patches for
the big chooser patch.
* Incorporate the feedback I give you on each single-purpose patch,
until it's of the quality needed for incorporation.

You've done the *hard work*. You've got the thing going. So please, why
the grief over the routine tuning to make it ready for incorporation?

Rob
-- 
GPG key available at: <http://users.bigpond.net.au/robertc/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://cygwin.com/pipermail/cygwin-apps/attachments/20030330/7549b358/attachment.sig>


More information about the Cygwin-apps mailing list