Re: RFC mailbox interface

[Jeffrey Stedfast <fejj stampede org>]

> Hi,
> Here is my second attempt at the API.  It incorporates some of the 
> suggestions made here as well as a mistake I made since I'm used to 
> writing C++.  So, how's this one.

I've got a few comments, so bear with me...

> 
> typedef struct _Message Message;
> typedef struct _Folder Folder;
> typedef struct _Server Server;
> 
> typedef struct _Flags {
>         int READ    :1;
>         int NEW     :1;
>         int TRASH   :1;
>         int DRAFT   :1;
>         int FLAGGED :1;
> } Flags;

I guess this is ok, might be better to just do guint32 flags; and then
have macros to set the corresponding bits, this way you could add more
flags at a later date without breaking compatibility. Not a huge deal
tho...this'll work.

> 
> //all feilds are read-only to clients
> struct _Message {
>         Folder *owner;
>         GList *views;
>         Flags flags;
> 
>         //these header fields are unfolded
>         char to[],from[],subject[],date[];
>         char msg_id[],references[],in_reply_to[];
>         char cc[],bcc[],reply_to[];
>          
>         gpointer data;
> };

I don't like this structure at all really. First, it should probably be
ref-counted, this'd save you a few interfaces that you designed below.
Secondly, if you are going to have a Message object, it should really do
it the Right Way (tm) and not stop at just the headers you tossed in
there along with a gpointer data. A better approach would be to design a
Message object that would represent the mime hierarchy. Also, you could
make it so that you could request *any* message header. This is probably
most easily done using a hash table or something. A better way to
represent the message body than "gpointer data" would be to have a tree
of mime parts, each mime part would then have it's own header hash table
as well (mime parts are kinda like mini-messages).

To ease appending a message to a folder, you should probably also make
it so that you can do Message::to_string() or something on it.

I kind of feel that a message shouldn't be tied to flags and whatnot,
but whatever. I later comment on why...

> 
> typedef struct _Stats {
>         int New;
>         int Draft;
>         int Flagged;
>         int Read;
>         int Total;
> } Stats;
>          
> typedef struct _Folder_Funcs {
>         void (*lock)(Folder*);

this should have error codes, what if you can't lock the mailbox?
I suggest:  int (*lock) (Folder*);

returns -1 on error and 0 on success or something. That keeps with most
of the libc stuff, if ya want to keep with a standard (which is a good
idea to do, otherwise you have to keep looking up return codes and
stuff).

>         void (*unlock)(Folder*);

this could also return error codes, but you could probably get by
without them (for the most part, an unlock shouldn't fail...?)

>         gboolean (*sync)(Folder*);

This is fine, but should probably decide on either gboolean or int
returns for all functions that can fail, rather than having some return
-1 and some return FALSE and stuff.

I think an int return is better, this way you can have a different value
for why it failed. <0 == failed, -1 = generic error, -2 = folder
invalid, -3 = server disconnected, etc...

>         //deletes all messages in folder
>         gboolean (*empty)(Folder*);

maybe call this expunge for consistency's sake (IMAP calls it EXPUNGE).

>         //only updates list if folder is open?
>         Stats (*check)(Folder*);
>          
>         //possibly creates Message list
>         //required for most functions
>         Stats (*open)(Folder*);

this won't work (you can't return a struct), I guess you could return a
struct pointer tho...

>         //possibly free Message list
>         void (*close)(Folder*);
> 
>         Message* (*getMessage)(Folder*, int msg);
>         // -1 fail, 0 success w/ folder closed, >0 msg index w/ folder
open
>         int (*addMessage)(Folder*, char data[], Flags flags);

I propose: int (*addMessage)(Folder*, Message*, Flags flags);

if you go with my recommendation that a Message not contain flags, but
if you choose to have the Message struct contains flags, then I suggest:

int (*addMessage)(Folder*, Message*);

Normally when you copy a message from one folder to another, you want it
to keep the flags.

>         int (*deleteMessage)(Folder*, Message *msg);

I think a better interface would be: int (*deleteMessage)(Folder*, int
sequence_id);

>         int (*alterMessage)(Folder*, Message *msg, const char data[]);

why would you ever want to alter a message?

>         int (*setFlags)(Folder*, Message *msg, Flags flags);

If you go with my suggestion about Message*, then:

int (*setFlags)(Folder*, int sequence_id, Flags flags);

If not, then why do you even need it? you can just set it directly on
the Message object.

>         // return unfolded headers?
>         GList (*getAllHeaders)(Folder*, Message *msg);

This method doesn't belong on a folder object, it belongs on the message
object.

>         char* (*getBody)(Folder*, Message *msg);

same as above.

>         char* (*getAll)(Folder*, Message *msg);

and again...

>         //using copy-in allows for vfolders (ref copy)
>         int (*copyMessage)(Folder*, Message *src);

if Message was ref-counted, you wouldn't need this. Again, I don't feel
this belongs on a folder object - it's an operation on a message.

>          
>         //IMAP format
>         GList (*findMessages)(Folder*, const char srch[]);
>         GList (*getHeaders)(Folder*, Message *msg,const char hdrs[]);
>         char* (*getBodyStructure)(Folder*, Message *msg);
>         char* (*getBodyParts)(Folder*, Message *msg, const char
parts[]);

I don't think findMessages should be restructed to just IMAP, if you
want vfolders this could come in handy for all mailbox types.

what is getHeaders() supposed to do? I'm confused...

getBodyStructure() should probably be an internal method for IMAP only
folder types and not a method on a generic folder object. I don't see
any real reason to have this anyway...

getBodyParts() should be a Message method and not a method on a folder

> } Folder_Funcs;
> 
> struct _Folder {
>         Server *server;
>         Folder_Funcs *funcs;
>         gpointer data;
> };
> 
> struct _Server {
>         Folder* (*getFolder)(Server*, const char path[]);
>         //should this return a Folder* for the new folder?
>         int (*newFolder)(Server*, const char path[]);
>         //return list of available Folder paths for getFolder
>         GList (*scanFolders)(Server*, const char path[]);
> 
>         //remote server functions
> 
>         gboolean (*logon)(Server*, const char user[], const char
passwd[]);
>         gboolean (*logoff)(Server*);
>         //sets path and size for cache use size=0 for unlimited
>         gboolean (*setcache)(Server*, const char path[], gsize size);
> 
>         gpointer data;
> };

I guess this ok...

Jeff

-- 
Jeffrey Stedfast
Evolution Hacker - Ximian, Inc.
fejj@ximian.com  - www.ximian.com