Re: [evolution-patches] Re: Improve folder treeview order

[Christian Neumair <chris gnome-de org>]

Am Mi, den 02.06.2004, 11:22 Uhr +0800 schrieb Not Zed:
> On Tue, 2004-06-01 at 21:18 +0200, Christian Neumair wrote: 
> > Christian Neumair schrieb: 
> > 
> > > The attached patch improves the sort order of folders inside the folder 
> > > treeview. See patch for order details. 
> > 
> > Sorry, the patch was not only lacking the most important part, it didn't 
> > work properly as well. Here comes the fixed patch. 
> Some comments below.  Like Jeff, i'm not that sure we want to do this;
> although I don't feel particularly strongly either way.

The reason I want to do this is that in locales != C, all the folders
are shuffled randomly, because they can be partly untranslated. If you
change the locale, you'll get a new shuffe. This looks heavily
inconsistent and causes confusion, because people remember "it was on
top", "it was the nth item", "it was the first item with a folder icon".
Ultimately, it looks ways more nice, see "[evolution-patches] Make
folder treeview internationalization work" [1].

> > Plain text document attachment (evo-treeview-order.diff)
> > Index: mail/em-folder-tree-model.c
> > ===================================================================
> > RCS file: /cvs/gnome/evolution/mail/em-folder-tree-model.c,v
> > retrieving revision 1.53
> > diff -u -r1.53 em-folder-tree-model.c
> > --- mail/em-folder-tree-model.c	28 May 2004 17:04:18 -0000	1.53
> > +++ mail/em-folder-tree-model.c	1 Jun 2004 19:17:12 -0000
> > @@ -177,6 +177,40 @@
> >  			      G_TYPE_STRING);
> >  }
> >  
> > +/* This makes folders show up in the following order: Inbox, Outbox, Drafts, Sent, Junk, Trash, Others */
> Not quite what it does, it puts Trash before Junk :)
> 
> Personally i'd put junk and trash last of all, but maybe thats just
> me.

I wasn't sure on the last two.


> > +static guint
> > +name_to_position (gchar *name)
> > +{
> > +	g_assert (name);
> Why add an assert?  The old code checked for name NULL, it shouldn't
> change behaviour.

why does one add treeview rows that have no label. That doesn't make
sense and in my opinion hints at broken code.

> > +	if ((!strcmp (name, "INBOX") ||
> > [...20 lines of junk...]
> > +		return 6;
> I will say one word, well two.  Tables.  Loops. :)  There's no reason
> at all for a nested if block here.

Ok, ok. I suck. I don't know why I used these stupid semantics :).

> I would probably use negative numbers for the indexes too, and 0 for
> not found/not special.  Fewer places to change if the list changes.
> 
> > +}
> > +
> >  static int
> >  sort_cb (GtkTreeModel *model, GtkTreeIter *a, GtkTreeIter *b, gpointer user_data)
> >  {
> > @@ -184,6 +218,7 @@
> >  	char *aname, *bname;
> >  	CamelStore *store;
> >  	gboolean is_store;
> > +	guint atype, btype;
> >  	
> >  	gtk_tree_model_get (model, a, COL_BOOL_IS_STORE, &is_store,
> >  			    COL_POINTER_CAMEL_STORE, &store,
> > @@ -207,11 +242,12 @@
> >  		if (bname && !strcmp (bname, _("UNMATCHED")))
> >  			return -1;
> >  	} else {
> > -		/* Inbox is always first */
> > -		if (aname && (!strcmp (aname, "INBOX") || !strcmp (aname, _("Inbox"))))
> > -			return -1;
> > -		if (bname && (!strcmp (bname, "INBOX") || !strcmp (bname, _("Inbox"))))
> > -			return 1;
> > +		atype = name_to_position (aname);
> > +		btype = name_to_position (bname);
> > +
> > +		if (atype < 6 ||
> > +		    btype < 6)
> > +			return (atype == btype ? 0 : atype > btype ? 1 : -1);
> I prefer the GNU if syntax:
>    blah
> || blah
> although for such a short expression i'd put it on the one line.
> Also its better to check atype < btype   atype > btype, since they're
> almost never going to be ==.

Indeed

regs,
 Chris

[1]
http://lists.ximian.com/archives/public/evolution-patches/2004-June/005651.html