This is the mail archive of the cygwin-apps@cygwin.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [SetupXP] The two styles for handling activation refusal


Gary R. Van Sickle wrote:
>> Robert Collins wrote:
>>> On Sat, 2003-07-19 at 23:40, Max Bowsher wrote:
>>>> Gary's current SetupXP patchset calls 2 member functions on page
>>>> activation: OnActivate (returns void), and OnAcceptActivation (returns
>>>> bool). I think this is unnecessarily messy. AFAICS, OnAcceptActivation
>>>> only exists to prevent the need to change the return type of the
existing
>>>> OnActivate function.
>>>>
>>>> I would very much prefer changing OnActivate to return bool, combining
>> the
>>>> purpose of both functions. Yes, this does require changes in all
derived
>>>> classes, but the changes are trivial, and the end result is a cleaner,
more
>>>> logical API.
>>>
>>> If its what I think you are talking about, I disagree.
>>>
>>> OnAcceptActivation was, IIRC, prompted to allow pages to accept or
>>> refuse activation. Refusing == don't display. Accept = display.
>>>
>>> OnActivation, is called on each activation, which only occurs
>>> post-accept checking.
>>>
>>> OnAcceptActivation can have default behaviour for the common case,
>>> reducing duplicate code over a conflated function that both activates
>>> and indicates whether it's willing to be activated.
>>>
>>> Naming wise though, I'd call OnAcceptActivation canBeActivated or some
>>> other query-indicating function.
>>>
>>> Again, all the above is based on recollection..
>>
>> Right. One detail though, at the moment, OnAcceptActivation is called
>> *after* OnActivate.
>> That *is* redundant.
>
> It is indeed completely redundant.
>
>> Unless there will ever be a need to ask a page whether
>> it would take activation in the future, but not activate it immediately,
>> even if it is possible to do so, I think the 2 calls should be merged.
Will
>> there ever be such a case?
>
> I cannot think of one.  It exists soley to give OnActivate a "default
return
> code".  It *can't* be called anywhere else, since in the general case,
> OnAcceptActivation won't know if it needs to refuse activation until after
> OnAccept is called.
>
> I'll let you guys fight this one out.  I've coded it up both ways, it
works
> both ways.  Rob's way results in fewer bytes of patch, a one-time hit that
I
> see as being of dubious value.  It adds a bit of abstraction above the
return
> code way, which may arguably be of value and may even reduce the binary by
a
> few bytes. The return code way OTOH probably speeds things up by a few
clock
> cycles. Personally I think it's five of one, half-dozen of the other.
>
> I'll just say this: check *one* of them in.  Arguing about these
trivialities
> is a silly waste of time that accomplishes nothing.  Check something in,
and
> then if somebody comes along with a third patch that pleases everybody,
then
> argue about that.  Meanwhile, we'd actually have some work done to setup.

Gary, I don't think the clarity of setup's code is trivial.
And, I can't imagine that any project would accept a monolithic patch
encompassing multiple concepts.

You've produced a very large contribution, but the way you have packaged it
makes it a lot of work to merge. Fortunately, I have lots of time at the
moment.

Right now, we have 3 seperate elements of your patch under active
discussion:
* Minor res.rc changes
* LogFile::exit
* PropertyPage::OnInit

I'm not going to be thinking about any more elements until some of these are
resolved, so repost your diff after every time you make any changes, and
discuss the above as much as necessary.

Max.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]