Re: [Evolution-hackers] NNTP patch

[Meilof <meilof wanadoo nl>]

Not Zed wrote:

> Well something broke.  When you reply to emails it brings up a 'post'
> composer with no folder set, and you can't turn off the post entry, and
> you have to turn on the to entry manually.  Something isn't quite right
> there.
Fixed.

> Also, when yuou send mails, they get put in the outbox but aren't sent
> immeediately, so somehting else broke there too.
Fixed too.

> - 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?

> - 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...

> - some sytlistic things in the patch
> dont do this:
> if () foo;
> do this instead:
> if ()
>  foo;
>
> +static enum { fail, error, success }
> +nntp_folder_download_message(CamelStream **retstream, CamelNNTPStore
> *nntp_store,
> +                             CamelNNTPFolder *nntp_folder, const char
> *msgid, CamelException *ex)
>
> i'd prefer this just returned an 'int' and used -1 for failure.
>
> +	if (url->user)
> +		fi->url = g_strdup_printf ("nntp://%s %s/%s", url->user, url->host,
> si->path);
> +	else
> +		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...

> +			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?

> +			camel_object_trigger_event (CAMEL_OBJECT (nntp_store),
> "folder_subscribed", fi);
>
> and similar camel-object methods, shouldn't be cast to CAMEL_OBJECT.
>
> Also as a rule we use direct C casts rather than macro casts on internal
> methods (although camel's casts are cheapish they're still more
> expensive and code-bulky).
>
> 		prompt = g_strdup_printf (_("Please enter the NNTP "
> +					    "password for %s %s"),
> +					  service->url->user,
> +					  service->url->host);
>
> generally merge strings like this onto one line, its not that long.
>
> +	/* Check for 'authentication required' codes */
> +	if (u == NNTP_AUTH_REQUIRED) {
> +		if (camel_nntp_try_authenticate(store)) goto command_begin_send;
> +	}
> +
>
> do this like:
>
> if (u == NNTP_AUTH_REQUIRED
>     && camel_nntp_try_authenticate(store))
> 	goto ....;
>
>
> EMsgComposer            *e_msg_composer_new
> (void);
> EMsgComposer            *e_msg_composer_new_post
> (void);
> +EMsgComposer            *e_msg_composer_new_mail_post
> (void);
>
> ^^ 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.

> static CamelMimeMessage *
> -composer_get_message (EMsgComposer *composer, gboolean post, gboolean
> save_html_object_data)
> +composer_get_message (EMsgComposer *composer, gboolean post, gboolean
> save_html_object_data, gboolean *no_recipients)
> {
> 	CamelMimeMessage *message = NULL;
> 	EABDestination **recipients, **recipients_bcc;
> @@ -296,10 +296,14 @@
> 	camel_object_unref (cia);
> 	
> 	/* I'm sensing a lack of love, er, I mean recipients. */
> -	if (num == 0 && !post) {
> -		e_notice ((GtkWindow *) composer, GTK_MESSAGE_WARNING,
> -			  _("You must specify recipients in order to send this message."));
> -		goto finished;
> +	if (num == 0) {
>
>
> Are you sure this change is right?  How could you ever setup a composer
> with no post or no to set?  One should be set?
Well, the complete patch says:

>         /* I'm sensing a lack of love, er, I mean recipients. */
> -       if (num == 0 && !post) {
> -               e_notice ((GtkWindow *) composer, GTK_MESSAGE_WARNING,
> -                         _("You must specify recipients in order to 
> send this message."));
> -               goto finished;
> +       if (num == 0) {
> +               if (post) {
> +                       if (no_recipients) *no_recipients = TRUE;
> +               } else {
> +                       e_notice ((GtkWindow *) composer, 
> GTK_MESSAGE_WARNING,
> +                                 _("You must specify recipients in 
> order to send this message."));
> +                       goto finished;
> +               }

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.

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


Meilof
-- meilof wanadoo nl