Re: [Evolution-hackers] Subclassable and extendable IMAPX

[Chenthill <pchenthill gmail com>]

On Tue, 2012-05-08 at 11:52 +0200, Christian Hilberg wrote:
> Hi everyone!
> 
> It has been a while [0] since the idea of making IMAPX
> subclassable / extendable for backends to use. Time to
> bring the topic back into the public again. :-)
> 
> There is a bugzilla entry [1] now for the topic, and Chen
> bravely started out with preparations to make IMAPX extendable.
> 
> We've been staring at the imapx_untagged() function in IMAPX
> for a while. This is the handler function which takes care of
> processing the untagged responses from the IMAP server, and it
> is the point where IMAP protocol extension code will want to
> start hooking into. There may be other hooks needed.
> 
> evolution-kolab uses IMAPX outside Evolution. Back in 2.30 era,
> IMAPX was not extendable. We thus had the IMAPX code duped and
> extended the hard way. Since I was overwhelmed by a single
> function imapx_untagged(), approaching 1k LOC in length, I tore
> it into pieces (one per untagged response) and added these into
> a function table. This collapsed imapx_untagged() to almost nothing,
> looking far more maintainable (and making the function extendable).
> For those interested in the approach, see [2].
> 
> When porting evolution-kolab from 2.30 to 3.4, I decided to try
> and subclass IMAPX in a clean way, since the 2.30 approach of
> directly hacking my code into an IMAPX dupe would turn it into
> a maintenance nightmare in the long run.
> 
> There are two kinds of extensions to IMAPX I'm doing in evolution-kolab:
> 
> * IMAP extensions (ANNOTATEMORE, METADATA (planned for),
>   IMAP ACL (planned for))
> 
> * Kolab extensions
> 
> While the latter is Kolab specific, the former is not at all Kolab
> specific, but holds IMAP extensions as per RFC. It therefore makes
> sense to strive for integration of these extensions into upstream
> IMAPX in order to have all users of IMAPX benefit from it. In the
> evolution-kolab source tree, the former resides in
> 
>   src/camel/providers/imapx
> 
> while the latter resides in
> 
>   src/camel
> 
> to make the distinction clear. There are a number of CamelIMAPXExtd*
> classes I've added to src/camel/providers/imapx which hold my
> extensions over the upstream IMAPX (the upstream files are duped with
> just minor changes, mostly exposing internal API for me to hook into).
> Please see [3] for the stretches I needed to make to subclass IMAPX.
> Main problem was that imapx_untagged() was not exposed, so I had to
> re-implement all code paths to that function inside my own extended
> classes. To ensure I would get my extended object types in the right
> places, I had to override the CamelIMAPXConnManager classes with my
> own, mainly because the IMAPX internal classes were not using virtualized
> functions (which made it impossible for me to just inject my types here
> and there - I had to make sure the right code paths were followed).
> The classes in src/camel (thr Kolab-specific ones) look somewhat alike,
> for that same reason, see [4].
> 
> I've not been diving into Chen's proposals very deeply yet. Allowing
> subclasses to override imapx_untagged() seems like the first step. I've
> not yet grokked all of the proposals, so I have a question: Will the
> current proposal (see [1]) allow for a subclassed IMAPX to be subclassed
> again? Subclassing IMAPX twice is what I'm doing in evolution-kolab.
> Of course, the first subclassing (for adding support for folder metadata
> and ACL) can just be dropped, once these extensions make it into
> upstream subclassable IMAPX.
> 
> Then, there's my table-based approach to imapx_untagged(). I've been
> having a chat with Matt about that (and back in the days when I did the
> implementation for 2.30, with David), which both liked the idea. Of course,
> my 2.30 implementation was not done with truly subclassing IMAPX in mind,
> so it would need some tweaking. I'll try to gather more thoughts as I'm 
> rovelling through the code of the proposal at BZ.
> 
> What do you think?
> 
> I would invite everyone who is hacking IMAPX to participate in this
> discussion.
I hope you had sent the mail just before our irc discussion. To
summarize, 
https://bugzilla.gnome.org/show_bug.cgi?id=674310#c9 has the fixes. We
now have a hook in imapx_untagged which would allow the subclasses to
handle the responses that are not handled in IMAP (imapx provider). I
did not find a case where a sub-classed imap provider would be
interested in tags which are already processed by the parent imap
provider, so i have provided a simple hook to handle, unhandled tags. I
hope this would be sufficient to support all the above needs.

Please let me know if you need anything more..

Thanks, Chenthill.
> 
> 
> [0] http://mail.gnome.org/archives/evolution-hackers/2010-September/msg00012.html
> [1] https://bugzilla.gnome.org/show_bug.cgi?id=674310
> [2] http://git.gnome.org/browse/evolution-kolab/tree/src/camel/providers/imapx/camel-imapx-server.c?h=gnome-2-30#n1363
> [3] http://git.gnome.org/browse/evolution-kolab/tree/src/camel/providers/imapx
> [4] http://git.gnome.org/browse/evolution-kolab/tree/src/camel/
> 
> _______________________________________________
> evolution-hackers mailing list
> evolution-hackers gnome org
> To change your list options or unsubscribe, visit ...
> http://mail.gnome.org/mailman/listinfo/evolution-hackers