Re: [Evolution-hackers] initial NNTP patch

[Not Zed <notzed ximian com>]

Cool!

But could you re-do the patch using -u3 to diff?  Patch isn't liking it and not applying it automatically.  I'd like to apply it and give it a test!

Also, first impressions, mostly just stylistic issues - read the HACKING file, you're using some things like c++ comments, gchar's etc for the basic types (at least in camel we prefer using the basic c types for basic c types), and not using k&r 'linux kernel' style indenting.

i.e.

>               CamelFolderInfo *i = (CamelFolderInfo*)g_ptr_array_index(groups, t);
>               if (!i) { return NULL; }
>               if (strcasecmp(i->full_name, name) == 0) return i;

should really be

CamelFolderInfo *info = ...;

if (info == NULL)
    return;

if (...)
    return info;

(also, don't use 'i' for other than loop variables, use 'fi' if you want for CamelFolderInfo, as is used in other places in the code).

(actually that whole method heir_find_folder is questionable, should probably use a hashtable, a linear search will get slow with a big list like a newgroup list).

On Tue, 2003-12-09 at 10:16, Meilof wrote:
> Hello,
>
> Just to get some review before I hear the _whole_ patch is messed up, I 
> proudly annouce this initial NNTP patch for Evolution:
>
>   http://home.wanadoo.nl/meilof/evolution-nntp-patch-04
>
> It does subscribing/unsubscribing and caching of the newsgroup list, and 
> it sorts the newsgroup list too. Please let me know whether I'm on the 
> right track.
>
> Also, couple of issues:
>
>     * Loading a complete newsgroup list still takes a considerable
>       amount of time, largely due to the sorting I have implemented. If

Won't it just take a lot of time anyway?  It could be a long list.  If you have a tree view (like in the subscribe dialogue), then you could list more incrementally (i can't remember, you can do a list of alt.% or something to get a given level without having to get everything?).

Its only real slow the first time isn't it?

>       you want to subscribe to the z.z.z newsgroup, this means you have
>       to wait quite some time. I propose that if you click on the "z"
>       folder of the "z.z.z" newsgroup, it will get updated directly
>       rather than after all previous folders, so that if you know what
>       you want to find, you can subscribe/unsubscribe easily.
>     * I think it would be kind of nice to have short folder names, like
>       "a.b.mp3" for alt.binaries.mp3. I did implement this functionality
>       in the NNTP store (a configuration setting of the newsgroup), but
>       this is obviously something that should be handled by the mailer
>       rather than the NNTP backend.

Good idea.  It could go at any level, the backend isn't such a bad spot to put it, how you've done it now, so long as it properly maps to the long names for subscriptions etc.

>     * The bounty description mentions the store summary having a "flat"
>       and "tree" mode. I just did a s/IMAP/NNTP of the
>       imap-store-summary, which seems to work currently, but this is
>       probably not what you indended. Can you tell me in concrete where
>       these flat and tree modes would be used withing Evo, and what kind
>       of functions need to be written to support that?

What i meant by this is either:

comp.sys.linux
comp.sys.linux.kernel
alt.rec.cycling

or
comp/
  sys/
    linux/
      kernel/
alt/
   rec/
     cycling

for the display mode in the tree view (you could of course collapse them if they have an empty parent).

I guess it would make sense to always do the latter for the subscription dialogue, and have it optional for the frontend, but the api doesn't really cater for alternatives since they both go through the same interface.

>     * For offline operation, would it be a good idea to subclass the
>       CamelDiscoSummary?
I don't think discosummary is needed (i can't remember what its for - maybe needed for posting, but maybe not, news posting is different/easier than imap), but you would need to subclass discostore and maybe discofolder.

I had hoped to work on changing those interfaces but didn't get around to it, so for now they should be used.

> I think that was pretty much it. The patch seems to work for me, and 
> I'll go over to subscribing to groups first now I think. I have posted 
> some screenshots (never mind the desktop environment I was running... :S) on

Send in a unified diff and i'll have a closer look at it!

Thanks!

 Michael