Re: [Logicmail-devel] Merge notes for code contributed by George Sexton

[Derek K. <dko...@lo...>]

Ok, I've decided to move this discussion to the list.  For everyone joining 
in, at the very bottom you'll see my merge notes from a contribution from 
George that I committed to the repository last night.  Then is his follow-up 
to it and plans going forward.

I'll address the four points of George's e-mail in-order now...

1) Refresh display window after a message delete or move
Yes, this definitely needs to be done, if it currently isn't working.  Though 
it may be something easier for me to deal with, unless you're brave enough, 
since it involves correctly handling (maybe adding) asynchronous events across 
the 3 layers of the app.  (UI, object-data-model, mail-protocol-state-machine)  
I'll take some serious focus to figure out the complete use-case of this, but 
it should have some vague similarities to what I did late last week to get the 
outbox working.

Of course move/copy in general needs a little bit of work/scrubbing, since 
there are two ways you can do it.  (download/upload or IMAP server-side, the 
latter being safer/less-likely-to-muck-data than the former)

But if you look at some of my to-do items in the code (which Trac does extract 
and show in the "Code Tags" section of the website), you'll see that there 
probably are several nuances to this that need to be worked out.
----

2) Launch attachments if there's a handler registered for the mime type.

While I'm not sure attachment-downloading is 100% done/robust, this specific 
item is something you could definitely work on.  So far I've only implemented 
attachment-saving, and want to keep that option.  I'd recommend making it 
another menu option, or selectable via a dialog, when an attachment is 
selected.
----

3) Look at some of the connection handling. Errors seem to throw the 
connection out and cause cascading issues.

This was actually the next item on my own to-do list, along with some 
intelligent code for actually opening network connections.  (and eventually, 
automated polling)

I know the request-driven state machine that handles mail server network 
connections doesn't have much in the way of intelligent error handling at the 
moment.  (The big exception is outgoing/SMTP, where I did implement error 
notification upstream.)  Its very easy to build up a lot of things on the queue 
without realizing it.  In theory, if the connection gets closed due to an 
error, it should flush the queue.  Of course in practice, I could see that 
maybe its not always happening as expected.

One thing I could definitely use from you is some good test cases that produce 
issues, which would help me iron out the bugs more quickly.
----

4) One thing I've thought of doing is a configuration wizard.

Already wrote one, believe it or not :-)
On the config screen, click on the Account node and select the "wizard" option.  
Of course the intent is for it to show automatically for new users, and for it 
to also incorporate a lot of auto-configuration things like you mention.  But 
at least I already put in all the hard work towards a wizard-function UI 
framework in there, and we can build off of that.

Of course the biggest up-front problem is making sure the user can actually 
make a network connection.  This problem gets easier in the 5.0 API, and 
different API versions have different features for inquiring network 
capabilities.  (RIM has a nifty open-source tool called "NetworkDiagnostic" 
that shows some of this)

Once these problems are solved (see #3), automatically figuring out the 
configuration (per your ideas) can become a reality.  You might also want to 
look into this, and Google anything related to it, for ideas on how to 
design/leverage such a capability:
https://wiki.mozilla.org/Thunderbird:Autoconfiguration

----

With this all being said, there is one thing I *really* need to find time to do 
right now.  Its something that often gets pushed asside, especially in F/OSS 
projects.  That is up-to-date design documentation.  The diagrams/descriptions 
in the design section of the website were all posted *before* I started 
serious coding on 2.0, and really haven't been updated since.  If I had this, 
it would be much easier for people to understand the software well enough to 
contribute to the more interesting parts.

The second thing I need, of course, is to find more opportunities to put 
serious time and effort into the code.  Last week was a huge exception, where I 
literally took a whole week off of work to do LogicMail nearly full-time.  I 
made tremendous progress, and finally got the code to the dogfood stage, but 
now I really need another one of those weeks.  (on some weeknights, I barely 
have enough spare time to respond to e-mails and/or do simple stuff, and many 
of my weekends aren't quite as free as they used to be)

-Derek

On Tuesday 05 January 2010 12:34:19 pm you wrote:
> Thanks. I know that as an outsider looking at the code for the first time,
> I'm not going to have the kind of knowledge that you do about the
>  structure. I also know nothing about IMAP protocol. I do appreciate your
>  taking the time to help me understand why you're rejecting things. It will
>  help me avoid making the same mistake.
> 
> 
> 
> The three things I want to look at doing next are:
> 
> 
> 
> 1)      Refresh display window after a message delete or move.
> 
> 2)      Launch attachments if there's a handler registered for the mime
> type.
> 
> 3)      Look at some of the connection handling. Errors seem to throw the
> connection out and cause cascading issues.
> 
> 
> 
> One thing I've thought of doing is a configuration wizard. What I envision
> is you pass in an Email address. The code would then do a MX lookup for the
> domain, and try to determine automatically the following:
> 
> 
> 
> Is the address domain name valid? During my testing, I put in the domain
> name wrong. A simple DNS lookup could catch that.
> 
> 
> 
> Outbound Server Name (lowest MX. If no MX, use Domain Name)
> 
> Inbound Server Name (lowest MX)
> 
> 
> 
> Outbound Port (Start w/ SSL ports, and work down)
> 
> Highest Supported Protocol/Port (IMAP/SSL, IMAP, POP3 SSL, POP3)
> 
> 
> 
> I'm more of a system guy than a UI guy so writing this kind of code would
>  be pretty simple for me. It would also be a drop-in to integrate.
> 
> 
> 
> Then, these become the defaults for the subsequent pages of account setup.
>  I had to set our youth pastor's droid up this weekend because he didn't
>  understand anything about the configuration for a mail client. If you can
>  make the program automatically figure it out for 80-90% of users it would
>  be a big win.
> 
> 
> 
> 
> 
> George Sexton
> MH Software, Inc.
> http://www.mhsoftware.com/
> Voice: 303 438 9585
> 
> 
> From: Derek Konigsberg [mailto:dko...@lo...]
> Sent: Monday, January 04, 2010 8:27 PM
> To: George Sexton
> Subject: Merge notes for code contributed by George Sexton
> 
> 
> 
> Here are my notes for the code I just merged in.  I apologize for this
>  being an HTML message, but it was the most painless way to paste it in
>  from my local notes app (Tomboy Notes).
> 
> Your contribution consists of two major pieces:
> A series of minor code-review-type changes, which were either merged as-is
> or with minor modifications.
> The much-desired subscribed folders feature, to which I added a
> configuration option before merging.
> 
> Contributor: George Sexton <
> <file:///C:\Users\octo\Desktop\ge...@mh...>
> ge...@mh...>
> RmsDataStore
> 
> *	Removal of "instanceof" check from load()
> 
> *	Agreed with rationale, but replaced with a null check instead of
> outright removal. Serialization errors could still return null, and
> instanceof was doing an implicit null check.
> 
> PersistentObjectDataStore
> 
> *	Removal of "instanceof" check from load()
> 
> *	Agreed with rationale, but replaced with a null check instead of
> outright removal. Serialization errors could still return null, and
> instanceof was doing an implicit null check.
> 
> StatusBarField
> 
> *	Minor change to if-statement login in setStatusText()
> 
> *	Accepted change as-is
> 
> OutboxMailboxNode
> 
> *	addMailSenderListener(): Change from postfix to prefix increment
> operator on mail sender count
> 
> *	Accepted change as-is
> 
> MessageNodeReader
> 
> *	read(): Improved robusness of code
> 
> *	Accepted change as-is
> 
> MaildirFolder
> 
> *	getMessageSourceFromStream(): Pre-allocate initial size of
> StringBuffer
> 
> *	Accepted change as-is
> 
> PopMessageToken
> 
> *	hashCode(): Minor change to remove unnecessary local
> 
> *	Accepted change as-is. Original version was probably a modification
> of something auto-generated by Eclipse, but the fix makes sense.
> 
> ImapProtocol
> 
> *	prepareFetchEnvelopeResponse(): Added comment about the case when
> parsedText is null
> 
> *	Put "continue" in exception handler to get desired
> skip-parsing-this-one behavior
> 
> *	executeList(): Refactored into executeList() and
> executeListSubscribed() with a common implementation
> 
> *	Renamed executeListSubscribed() to executeLsub() to fit naming
> convention of public methods in ImapProtocol
> 
> *	executeListVerb(): Added check for flagStr being null. Made comment
> about the case where argStr is null.
> 
> *	Renamed method to executeListImpl()
> *	Modified code to instead skip all parsing if either flagStr or
> argStr are null, since either being null means the server violated the
> response format documented in the RFC.
> 
> ImapClient
> 
> *	open(): changed LIST call to LSUB
> 
> *	Rejected change. In this specific instance, LIST is used as a
> workaround for servers that do not supply valid namespace information. It
>  is used to get the folder delmiter character. Calling LSUB with empty
>  parameters does not have the same behavior, and thus does not work here.
> 
> *	getFolderTreeImpl(): changed LIST call to LSUB
> 
> *	Accepted with the change that ImapConfig.getOnlySubscribedFolders()
> is first checked and the old behavior is still used if configured to do so.
> (see below for details)
> 
> *	getMessageStructure(): Changed active mailbox sanity check
> 
> *	Modified to test for null instead, like all the other sanity checks,
> since that is the likely situation if an active mailbox has not been set.
> 
> *	getMessageBody(): Changed active mailbox sanity check
> 
> *	Modified to test for null instead, like all the other sanity checks,
> since that is the likely situation if an active mailbox has not been set.
> 
> Non-patch-provided changes: ImapConfig
> 
> *	Added onlySubscribedFolders field, from the 1.1 branch, but with a
> default of true.
> 
> AccountConfigScreen
> 
> *	Added a checkbox labeled "Show only subscribed folders" to the
> advanced page for IMAP configurations
> 

-- 
---------------------------
Derek Konigsberg
dko...@lo...
---------------------------