[PATCH 1/4] Re: Do away with UI plugins and simplify calling the GUI

[Leif Walsh]

On Fri, 24 Dec 2010, Nicolas Sebrecht wrote:

> On Thu, Dec 23, 2010 at 01:06:16PM -0500, Leif Walsh wrote:
> > On Wed, Dec 22, 2010 at 9:27 AM, Sebastian Spaeth <Sebastian at sspaeth.de> wrote:
> > 
> > > Blinkenlights was not optimal for various reasons, e.g. Exceptions
> > > would be hidden as the screen immediately reset itself after exit. It
> > > also frequently messed up the screen when offlineimap crashed.
> > 
> > Please don't completely remove a feature because you don't like it or
> > think it has a couple bugs.  The typical way to handle these things is
> > to either fix it or file a bug report so someone else can, and if
> > nobody does for a few months or a year, you can mark it deprecated and
> > remove it after another year or so.
> 
> This has already been discussed. We came to the conclusion that most UI
> aren't used at all and don't help reducing the maintenance code. This is
> why we decided to simply remove some UI. I think it's better to NOT have
> broken UI than keeping them along.
> 
> Because of the feature changes, I don't plan to merge such a patch
> before the next major release (we could go from v6.3.x to 6.4.0).
> 
> But you don't say what looks more critical to me: do you really use the
> Blinkenlights UI? In which use case another UI can't do the job?

Yeah, I do use it.  Of course I can sit there and watch some other thing
scroll up the screen, it's not a huge deal, it just seems bold (at least)
for a project to blow away a feature suddenly like this.  I must have
missed the discussion on this proposal, how long ago was it?

> > > Simplify the names of the GUIs to something that a mortal can actually
> > > remember, make them "TTY, Basic, Quiet, Machine" with TTY being the
> > > default.
> > 
> > You are introducing changes that will break existing configuration.
> > Have you documented this in enough places?
> 
> The documentation is currently frozen for the time I finish the SGML to
> RST migration. This is a work in progress. I hope to submit and then
> merge it before the next stable release.

Ok.

> > Have you documented this in enough places?  Does the code fail
> > gracefully given an old config?  Echoing Nicolas, this patch needs to
> > be broken up into smaller pieces, in particular so that I can answer
> > these questions for myself by reading the code.  With a patch this big
> > affecting this many pieces of code and features, I don't want to look
> > through it to figure these out.
> 
> I didn't read the patch sent for the same reason (not because the goal
> is not interesting). Will see in the next round.
> 
> > It also looks like you have a bunch of "code style" changes sprinkled
> > in.  It is best (though I understand, annoying) to separate these from
> > "application logic" changes, for the same reason.  I would rather not
> > read every line of your patch to determine what it does, it would be
> > nicer if every line in a patch could be a change to make the same one
> > thing happen, because that's the topic.
> 
> Some code style changes are good to include to other patches while
> touching an area of the code (typically when the style change affects
> the same code you're touching). I don't like pure code style patches
> that much because of the risk to include new failures. Patches
> introcucing failures (perhaps even tested) because of code style only
> changesets are good to avoid.  I can't say because I didn't read the
> patch of Sebastian.
> 
> That said, I won't be here as strict as I am (or as you will hit) in
> other projects: the active developers community around OfflineIMAP is
> not so wide.
> 
> But you should tell exactly what you have in mind by quoting the
> changesets. I ask authors to send patches by email for good reasons, and
> one of the main reason is to help reviewers to comment changes EASILY.

There was I think in patch 3/4 or 4/4 a bunch of changes to the way
modules are imported and accessed (fewer additions to the local module
namespace, more accesses within the code to offlineimap.something).  When
it's done to code that is changed anyway that's fine, but reading each
line to figure out if it's a feature change or if the developer just
decided to change a variable name in this line, for example, is a huge
pain for review.

> Sebastian do send his patches by mail. It would be very appreciated to
> not ignore this workload he did _for us_.

Yes, thank you Sebastian.  Keep up good work.

-- 
Cheers,
Leif