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

[Not Zed <notzed ximian com>]

Hi there,

I've been holding off on this for a while ... part of the reason is now
Srini is working on re-doing the attachment bar code significantly, so
when that is done the patch wont work without a rework.  And I didn't
know what to say ...

I'll comment on it anyway for the sake of pointing out good and bad
bits.

> NotZed wrote : 
> > The original is better than the last one or this one.
> > 
> > The problem is when discussing code there's always a lot of ways to do
> > the same thing.  Maybe it is an experience thing, and it isn't always
> > good - I often see so many ways to solve even simple problems that I'm
> > blinded from the more straightforward ones.  So in email or irc what
> > comes out is often a rush of partially thought-out possibilities, rarely
> > the ideal path, and sometimes not even a reasonable path.
> 
> > So I apologise if the various discussions just get you confused, they're
> > often thought experiments which aren't meant to go further, rather than
> > actual solutions.
> 
> Its totaly cool, i guess everyone has different ways of interpeting
> speach/chatting/writing/etc and that one has to interpet things
> different from different people. Say with some i might have to sort
> away lots of things beeing said, and read between the lines, but where
> with some one might not have to...
> 
> Also i guess it differs from the different kind of topics one discuss.
> Its probably true, that speaking solutions of code might sortof be
> hard. Seeing how one problem can be solved in various different
> sollutions, probably one solution per person.
> 
> That is the beauty of the development bussness, aint it? :)

Well its only beautiful if everyone's on the same page, which is rare.
Otherwise it is very frustrating ...

> > > something like this perhaps then, i had to re-use the size_to_string()
> > > function aswell, since it was a private one too.
> > 
> > Eek no, definitely not.  Having duplicated structures around is
> > definitely a recipe for disaster.  Its also full of c99 stuff like
> > in-line variable declarations which are right out, and it still leaks
> > the output of size_to_string (at least).
> > 
> > Ugh, who on earth wrote all that 1e3L mess, hmm, messy.  Whats so
> > unreadable about "1024".  *shakes head*  But I digress ...
> 
> jup, its much easier to understand if it would have said "size <
> 1024" instead of how it is now.
> 
> i'd probably do it something like :
> 
> gchar *
> e_msg_composer_attachment_size_to_string (gulong size) {
>         gchar *size_string = NULL;
>         gdouble displayed_size;
> 
>         if (size < 1024) 
>             size_string = NULL;
>         else {
>                 if (size < 1024*1024) {
>                         displayed_size = (gdouble) size / 1024;
>                         size_string = g_strdup_printf(_("%.0fK"),
> displayed_size);
>                 } else if (size < 1024*1024*1024) {
>                         displayed_size = (gdouble) size / (1024*1024);
>                         size_string = g_strdup_printf(_("%.0fM"),
> displayed_size);
>                 } else {
>                         displayed_size = (gdouble) size / (1024*1024*1024);
>                         size_string = g_strdup_printf(_("%.0fG"),
> displayed_size);
>                 }
>         }       
>         return size_string;
> }
> somewhat easier to read. imo.

I would probably not even bother with the temporary displayed_size
variable, it is only used once, may as well just do it in the function
call.

Because I hate floats i'd probably do it somewhat differently anyway,
you're just stripping off any values after the decimal point, so i'd
probably just use integers.

> Though, the smoothest way would probably be to have a function that
> just did set the lable, having a 2 parameters: The lable to set and

its "label" btw :)

> the list of attachments. Then in it you would have had the
> size_to_string function sortof. That way you could set the value on
> the lable and then free the char*. Right?
> 
> Something like the above function but doing it a "static void" instead,
> instead of ending it with a "return" you'd have it set the lable. Then
> free / clean up ...

That would work - but then you have a very specific function you can't
use for anything else.  (whether that is important here is somewhat
debatable).

> > I have an easier solution:
> > 
> > Convert the size_to_string in to a public static method on
> > e-msg-composer-attachment(or -bar) (i.e. just make it an external
> > function, say e_msg_composer_attachment_size_to_string).  You dont need
> > to change anything but its name (this is more or less what you had in
> > the first patch right?).
> 
> Um in the first patch, i had a separate function wich was named
> "get_attachment_size_string" and returned the appropriate size +
> 'K'/'M'/'G' in e-msg-composer-attachment-bar.c wich was the reason i
> didnt need to re-define the size_to_string() function, it was called
> in the get_attachment_size_string() function giving the right return
> string.
> If i convert size_to_string() in e-msg-composer-attachment-bar.c i'd
> really only need to remove the 'static' infront of it. Though,
> that might screw things up for other functions using it.. hmm..
> > Add a single public method to e-msg-composer-attachment-bar 'get_size'
> > which calculates the size of all attachments.
> > 
> > Then, make the new code use those (fixing leaks).
> hm do i get you right? Create two public functions, one in say
> e-msg-composer-attachment, that is a copy of the size_to_string()
> function, and another one to calculate the total attachment size, that
> one in -attachment-bar.

Yes that is right.

> then in e-msg-composer.c i use the above functions in the
> attachment_bar_changed_cb() function. Right?
> 
> something like this, then, perhaps?!
> ---- cut ----
> 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 -u -p -r1.99 e-msg-composer-attachment-bar.c
> --- composer/e-msg-composer-attachment-bar.c    23 May 2005 07:10:03
> -0000     1.99
> +++ composer/e-msg-composer-attachment-bar.c    11 Jul 2005 21:22:24
> -0000
> @@ -115,8 +115,40 @@ size_to_string (gulong size)
>         return size_string;
>  }
> 
> +gchar *
> +e_msg_composer_attachment_size_to_string (gulong size) {
> +       gchar *size_string = NULL;
> +       gdouble displayed_size;
> +
> +       if (size < 1024)
> +           size_string = NULL;
> +       else {
> +               if (size < 1024*1024) {
> +                       displayed_size = (gdouble) size / 1024;
> +                       size_string = g_strdup_printf(_("%.0fK"),
> displayed_size);
> +               } else if (size < 1024*1024*1024) {
> +                       displayed_size = (gdouble) size / (1024*1024);
> +                       size_string = g_strdup_printf(_("%.0fM"),
> displayed_size);
> +               } else {
> +                       displayed_size = (gdouble) size /
> (1024*1024*1024);
> +                       size_string = g_strdup_printf(_("%.0fG"),
> displayed_size);
> +               }
> +       }
> +       return size_string;
> +}
>  /* Attachment handling functions.  */
> 
> +gulong
> +get_size (EMsgComposerAttachmentBar *bar) {
> +       gulong size = 0;
> +       GList *p;
> +
> +       for (p = bar->priv->attachments; p != NULL; p = p->next)
> +               size = size + E_MSG_COMPOSER_ATTACHMENT(p->data)->size;
> +
> +       return size;
> +}
> +
>  static void
>  free_attachment_list (EMsgComposerAttachmentBar *bar)
>  {
> 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 -u -p -r1.18 e-msg-composer-attachment-bar.h
> --- composer/e-msg-composer-attachment-bar.h    23 May 2005 07:10:03
> -0000     1.18
> +++ composer/e-msg-composer-attachment-bar.h    11 Jul 2005 21:22:25
> -0000
> @@ -71,7 +71,8 @@ void e_msg_composer_attachment_bar_attac
>  void e_msg_composer_attachment_bar_attach_mime_part
> (EMsgComposerAttachmentBar *bar, CamelMimePart *part);
>  int e_msg_composer_attachment_bar_get_download_count
> (EMsgComposerAttachmentBar *bar);
>  void e_msg_composer_attachment_bar_attach_remote_file
> (EMsgComposerAttachmentBar *bar,const gchar *url);
> -
> +gchar *e_msg_composer_attachment_size_to_string (gulong size);
> +gulong get_size (EMsgComposerAttachmentBar *bar);
> 
>  #ifdef __cplusplus
>  }
> Index: composer/e-msg-composer.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/composer/e-msg-composer.c,v
> retrieving revision 1.512
> diff -u -p -r1.512 e-msg-composer.c
> --- composer/e-msg-composer.c   23 Jun 2005 09:11:06 -0000      1.512
> +++ composer/e-msg-composer.c   11 Jul 2005 21:22:28 -0000
> @@ -120,6 +120,7 @@
>  #include "mail/em-menu.h"
> 
>  #include "e-msg-composer.h"
> +#include "e-msg-composer-attachment.h"
>  #include "e-msg-composer-attachment-bar.h"
>  #include "e-msg-composer-hdrs.h"
>  #include "e-msg-composer-select-file.h"
> @@ -143,6 +144,14 @@ typedef struct _EMsgComposerPrivate {
>         GtkWidget *load;        /* same for load - not used */
>  } EMsgComposerPrivate;
> 
> +struct _EMsgComposerAttachmentBarPrivate {
> +       GtkWidget *attach;      /* attachment file dialogue, if active
> */
> +
> +       GList *attachments;
> +       guint num_attachments;
> +       gchar *path;
> +};

^^ dont really (or really dont) want to copy the private structure here.

>  enum {
>         SEND,
>         SAVE_DRAFT,
> @@ -212,6 +221,7 @@ static void handle_multipart_signed (EMs
>  static void set_editor_signature (EMsgComposer *composer);
> 
> 
> 
> +
>  static EDestination**
>  destination_list_to_vector_sized (GList *list, int n)
>  {
> @@ -2303,10 +2313,14 @@ attachment_bar_changed_cb (EMsgComposerA
> 
>         guint attachment_num =
> e_msg_composer_attachment_bar_get_num_attachments (
>                 E_MSG_COMPOSER_ATTACHMENT_BAR
> (composer->attachment_bar));
> -       if (attachment_num) {
> -               gchar *num_text = g_strdup_printf (
> -                       ngettext ("<b>%d</b> Attachment", "<b>%d</b>
> Attachments", attachment_num),
> -                       attachment_num);
> +
> +       if (attachment_num) {
> +               gchar *num_text = g_strdup_printf (
> +                       ngettext ("<b>%d</b> Attachment, %s",
> +                       "<b>%d</b> Attachments, %s total",
> +                       attachment_num),attachment_num,
> +
> e_msg_composer_attachment_size_to_string(get_size(bar) ));
> +

You still need to free the string returned by size_to_string.

Anyway, as i said the attachment bar is changing radically, but the good
news (i think) is that most of this code should still be usable with it,
just the structure names and some of the access apis will change.

 Michael