This is the mail archive of the 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

OK, this is a general reply to multiple messages.

I still believe "bool OnActivate()" to be the better option - here's why:

The "if(canActivate()){OnActivate()}" scheme makes 2 method calls where only
one is required. It also opens the possibility for OnActivate to be called
when activation is not desired. The top of each OnActivate has to grow an
"if (!canActivate) return;", or trust its caller not to do something

I do not see "bool OnActivate()" as being confusing, nor as less intuitive
that firing 2 event handlers consecutively. It is true that every instance
of OnActivate has to start returning a value, but I don't see this as

Quoting Robert:
> Changing OnActivates meaning would conflate things in the method, and
> make the code harder to read - "what does false mean - that Activate
> failed?".

I think activating, or deciding not to activate, are tightly bound
activities, which *should* be handled together. And yes, false does mean
that Activate failed - because the page's internal logic chose to make it

Quoting Robert's Pro and Con points:
Pro1: * Factor out conflated functionality
Pro2: * Step us toward some form of MVC pattern
Pro3: * Improve readability

Con1: * Add additional logic to an extant method.
Con2: * Conflate GUI logic with processing logic.
Con3: * Make setup harder to read.

Addressing point-by-point:

Pro1, Pro3, Con3 : IMO, OnActivate is the logical and intuitive place to put
this functionality.

Pro2, Con1: IMO, the extra logic truly belongs here, and any other placement
contravenes Pro3, Con3.

Con2: As a piece of GUI logic, the page class is an appropriate location.
If, in the distant future, we gain an interactive text UI, then there would
be reason to factor code out into a new style-neutral UI layer - but that's
a whole new topic.

Right, that was my attempt to sell the "bool OnActivate()" style, which I
believe to be best. However, I do want something to get into setup *soon*,
so, if this hasn't convinced you, Robert, I will set about extracting the
minimal set of AcceptActivate() changes from Gary's monster-patch.


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