RE: [PATCH] Bigger Chooser 2

["Gary R. Van Sickle" <g dot r dot vansickle at worldnet dot att dot net>]

> This is the first-take review:
>
> rectcc.h contains a class RECTPP. I *think* I posted my preference here
> for file names - that is after the class.
>

I don't recall that, though I certainly could have missed it.

> 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?  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.

> 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.

> 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
);

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

> 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? ;-)

> 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.  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.  That was the problem it was intended to solve.
It wasn't intended to be the best possible way in history to do this.  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.

> 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.

> I'm stopping here.

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

> There is enough that will need altering, it's
> confirmed my opinion.
>

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.

> This won't go in as is.

Am I then left to *guess* what you're going to accept?

> It also won't go in as a uber-patch.

"uber" meaning what now?  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?

> The different things being done
> need to be split up, so that we can *sensibly* discuss changing them
> without you having to reissue a ~50K patch every time.
>
> 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.

> Simply
> because there was high latency is not a good reason to make an
> *assumption* about how I would have asked you to submit it.

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.

>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.  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".

--
Gary R. Van Sickle
Brewer.  Patriot.