Re: [Evolution-hackers] NNTP patch

[Not Zed <notzed ximian com>]

> >
> Fixed.
> >
> Fixed too.

Sweet.

> > - depending on the order you subscribe to folders, you can get a tree
> >being displayed in the folder list, including duplicates.
> >
> >e.g.
> > test.a
> > test.a.b
> > test.a.c
> >
> >if you subscribe to test.a.b then test.a, i think it does this
> >(frommemory).
> >
> Hmmm... I do not seem to have this problem, at least not with this 
> sequence and some other sequences I tested with. Actually, the folder 
> view shoudn't do a tree at all. Can you give another sequence that gives 
> this problem?

Ok when i've rebuilt i'll see if i can reproduce.

> > - you should use the CAMEL_FOLDER_NOSELECT flag on non-selectable/
> >nonsubscribable folders returned from get_folder_info(), rather than
> >popping up an error box if you try to subscribe to them, the
> >subscription dialogue will automatically not let you subscribe to them.
> >
> Allright, I now set the flag, but taking a brief look at 
> em-subscribe-editor, it doesn't actually seem to use it... I looked in 
> the sub_fill_level function where new nodes are added, but it doesn't do 
> anything of this kind. In fact, the whole word CAMEL_FOLDER_NOSELECT is 
> not present in the subscribe editor...

Hmm, good point :)

Hmmm.  Now i think about it, I think with IMAP it doesn't care, you can
subscribe to such folders.  And if we make the code honour it, it could
break subscribing to certain imap trees.  Not sure here, maybe the error
box will have to do for now.

> >+		fi->url = g_strdup_printf ("nntp://%s/%s";, url->host, si->path);
> >
> >This should probably use camel_url_to_string stuff, otherwise components
> >may not be properly encoded.
> >
> Yeah... IIRC the code was a FIXME where I got it from (the Imap store I 
> think), too.... Anyway, I made it use camel_url_to_string now...

Cool, yeah imap needs some love.

> >+			fi = nntp_folder_info_from_store_info(store->do_short_folder_notation, url, si);
> >  
> >
> >+			fi->flags |= CAMEL_FOLDER_NOINFERIORS | CAMEL_FOLDER_NOCHILDREN;
> >+			if (!fi) continue;
> >
> >you check fi after dereferencing it here.
> >
> Hmm... yes...
> 
> >+static CamelFolderInfo *
> >+nntp_store_get_cached_folder_info(CamelNNTPStore *store, const char
> >*_top, guint flags, CamelException *ex)
> >+{
> >
> >probably prefer to use intop or something rather than _top.
> >
> Why?

We don't use underscores before names anywhere.  And if i remember
correctly there's some C/C library rule that says underscores before
names are 'reserved for the implementation'.

> > 
> >^^ i wonder if it should just have 1 entry point with a type field.
> >Each function is almost identical apart from an internal parameter
> >change.
> >
> Yeah, I thought about that too, but I didn't think it would be quite my 
> place to modify the API like that. Anyway, I made a new 
> e_msg_composer_new_with_type, which is called by the default constructor 
> e_msg_composer_new.

Nod, thanks.

> > +               }
> 
> So, if the post flag is not set, then the same thing happens: you get 
> the e_notice. The only thing that changes is that when the post flag 
> /is/ set, and the caller wonders whether there are any mail recipients 
> as well (if so, it should post the message to the outbox), it set some 
> flag. This function doesn't actually determine where to post to, that's 
> done by the caller.

Ahh my bad, i kinda rushed through the patch.

> Anyway, did some stylistic fixes (didn't do the casts yet though -- 
> that'll follow), and removed the bugs you mentioned (except the problem 
> with subscribe/unsubscribe giving a tree -- I couldn't reproduce that):
> 
> New patches:
> 
>     * home.wanadoo.nl/meilof/evolution-nntp-patch-13
>     * home.wanadoo.nl/meilof/evolution-nntp-patch-12to13

Cool, more comments once i've given it a burn.

 Z