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

[smurfd <smurfd smurfnet homelinux net>]

Hey!

Using the cvs diff function, was indeed easy.

Im hoping the sewage has atleast turned into a small fork or
something ;)

I choosed to keep the get_attachment_size_string() function, due to the
fact that when trying to access a bar->priv->attachments in
e-msg-composer.c (even though, e-msg-composer-attachment-bar.h is
included in it) i did get some error complaining that i couldnt do that
way... dont remember exactly what the errormessage was...

I did a "patch -p0 < file.patch" in my evolution/ source dir, all went
fine. Updated from anon cvs a couple of hours ago, so it Should apply
cleanily.

regards 
/Nicklas

---- patch ----
? composer/.e-msg-composer.c.swp
? composer/mail-composer.error
Index: composer/e-msg-composer-attachment-bar.c
===================================================================
RCS
file: /cvs/gnome/evolution/composer/e-msg-composer-attachment-bar.c,v
retrieving revision 1.99
diff -r1.99 e-msg-composer-attachment-bar.c
403a404,413
> gchar* get_attachment_size_string (EMsgComposerAttachmentBar *bar) {
>       GList *p;
>       gulong size = 0;
>
>       for (p = bar->priv->attachments; p != NULL; p = p->next)
>               size = size + E_MSG_COMPOSER_ATTACHMENT(p->data)->size;
>
>       return size_to_string(size);
> }
>
Index: composer/e-msg-composer-attachment-bar.h
===================================================================
RCS
file: /cvs/gnome/evolution/composer/e-msg-composer-attachment-bar.h,v
retrieving revision 1.18
diff -r1.18 e-msg-composer-attachment-bar.h
74c74
<
---
> gchar* get_attachment_size_string (EMsgComposerAttachmentBar *bar);
Index: composer/e-msg-composer.c
===================================================================
RCS file: /cvs/gnome/evolution/composer/e-msg-composer.c,v
retrieving revision 1.512
diff -r1.512 e-msg-composer.c
122a123
> #include "e-msg-composer-attachment.h"
2303d2303
<
2305a2306,2307
>       GList *p;
>
2308,2311c2310,2315
<                       ngettext ("<b>%d</b> Attachment", "<b>%d</b>
Attachments", attachment_num),
<                       attachment_num);
<               gtk_label_set_markup (GTK_LABEL
(composer->attachment_expander_num),
<                                     num_text);
---
>                       ngettext ("<b>%d</b> Attachment, %s",
>                       "<b>%d</b> Attachments, %s total",
>
attachment_num),attachment_num,get_attachment_size_string(bar));
>               gtk_label_set_markup (GTK_LABEL(
>                       composer->attachment_expander_num),num_text);
>

---------------



On Thu, 2005-06-23 at 14:10 +0800, Not Zed wrote:
> 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
> 
> _______________________________________________
> evolution-hackers maillist  -  evolution-hackers lists ximian com
> http://lists.ximian.com/mailman/listinfo/evolution-hackers