Re: [Evolution-hackers] Patch proposal to e-msg-composer and -attachment-bar



Hi there,

To create a diff - either use CVS, i.e. check out the cvs code (anon cvs
works just fine), make your changes, and then do 'cvs diff composer -Nup
> file.diff'.  This is definitely preferrable and easiest - particularly
since things change so quickly patches against tar sources are usually
out of date in no time.

The other alternative is you need to copy the source tree/subtree, have
one un-touched and one touched version, and then use 'diff -u3 -r
orig/composer new/composer >file.diff' to create the patch.

On the code, you have way too many unecessary casts (no need to cast a
gulong to a gulong to add it to a gulong, or cast a function returning
gchar * to a gchar *, even though the function you're calling actually
takes a const char * anyway ... and no, dont cast to that either!), you
leak almost all the strings you allocate, and you're using ngettext
incorrectly.  Dont pass markup to gettext if possible (i guess this case
is unavoidable), and if you do, make sure you pass simple strings - not
functions.  And certainly dont pass it allocated strings that never get
freed!

i.e.
DO!
text = g_strdup_printf(ngettext("<b>%d</b> Attachment (%s)", "<b>%d</b>
Attachments (%s total)", attachment_num), attachment_num, size_string)

DONT!
text = g_strdup_printf(ngettext(g_strdup_printf(""),
g_strdup_printf(""), n));

AFAICT, apart from leaking like a sieve, the last method can not even
work properly since the gettext scanner wont be able to find the string
to use as the translation key, and even if it guessed the quoted string,
the key is only created at run-time so it couldn't possibly match it to
anything.

So no ... Srinivasa, the code isn't even close to ok, sorry!  Must look
closer next time!

The idea seems fine though, and the code is easy to fix.

Make sure you make a proper patch once it is ready too.

 Michael

PS i dont think you need a separate get_attachment_size_string function
either, it is only used in that one place and is a small loop that could
be written in a total of 2 lines of code.

On Wed, 2005-06-22 at 22:24 +0200, smurfd wrote:
> Hey! 
> 
> I have created a small addition to the attachment bar in the mail
> composer.
> What it does, is create a additional string to the "1 Attachment" / "2
> Attachments" lable in the mail composer attachment bar.
> Now it says, "1 Attachment, size of 5k" / "2 Attachments, total size of
> 100k"
> 
> I Personally think its a very handy thing to have, seeing how some SMTP
> servers has a limit on how large your mails can be (i was tought
> recently). If you have N attached files, it can be messy to know how
> much you have attached to the mail.
> 
> was unsure how to create a patch for the 3 of the different files, into
> 1 patch, and since you cant put files on the list, i'll try to describe
> where the things are supposed to be ;)
> 
> Best regards
> /Nicklas
> 
> --evolution/composer/e-msg-composer-attachment-bar.h-- 
> line 74: gchar* get_attachment_size_string (EMsgComposerAttachmentBar
> *bar);
> -----
> 
> --evolution/composer/e-msg-composer-attachment-bar.c--
> line 404: gchar* get_attachment_size_string (EMsgComposerAttachmentBar
> *bar) {	
> 	EMsgComposerAttachmentBarPrivate *priv;
> 	GList *p;
> 	gulong size=(gulong)0;
> 	
> 	priv = bar->priv;
> 	
> 	for (p = priv->attachments; p != NULL; p = p->next) {
> 		EMsgComposerAttachment *attachment;
> 		
> 		attachment = E_MSG_COMPOSER_ATTACHMENT (p->data);
> 			
> 		size = size + (gulong)attachment->size;
> 	}
> 	return size_to_string(size);
> }
> 
> ----
> 
> --evolution/composer/e-msg-composer.c--
> line 2308: 	if (attachment_num) {
> 		gchar *num_text = 
> 		g_strdup_printf ( ngettext( (gchar *)g_strdup_printf 
> 			("<b>%d</b> Attachment, size of %s",(int)attachment_num,
> 			get_attachment_size_string((EMsgComposerAttachmentBar *)
> 			composer->attachment_bar)),
> 			
> 			(gchar *)g_strdup_printf
> 			("<b>%d</b> Attachments, total size of %s",
> 			(int)attachment_num, get_attachment_size_string(
> 			(EMsgComposerAttachmentBar *)composer->attachment_bar))	
> 			,attachment_num));
> 		
> 		gtk_label_set_markup (GTK_LABEL(
> 			composer->attachment_expander_num),num_text);
> 				
> 		g_free (num_text);
> 
> 		gtk_widget_show (composer->attachment_expander_icon);
> 		
> 	} else {
> ... (same as the old stuff)
> ----
> 
> _______________________________________________
> evolution-hackers maillist  -  evolution-hackers lists ximian com
> http://lists.ximian.com/mailman/listinfo/evolution-hackers




[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]