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

[Christian Neumair <chris gnome-de org>]

Am Freitag, den 04.06.2004, 09:34 +0800 schrieb Not Zed:
> 
> The only thing i'd suggest is you're changing the assert logic still.
> The code doesn't use an assert to test for no name, it treats it as a
> valid condition - whether its right or wrong thats what it does.
> 
> The names (the raw ones) should be based on the proper camel names,
> trash and junk and unmatched have #defines for this.  And the
> translation name for unmatches is Unmatched, not UNMATCHED.
> 
> Otherwise it looks basically right.

OK, thanks for your suggestions! I've changed that.

regs,
 Chris

> On Thu, 2004-06-03 at 16:17 +0200, Christian Neumair wrote:  
> > Am Mi, den 02.06.2004, 19:35 Uhr +0800 schrieb Not Zed:
> > > On Wed, 2004-06-02 at 12:11 +0200, Christian Neumair wrote:  
> > > > 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].
> > > 
> > > How many people really change their LC_MESSAGES all the time?  If the
> > > answer isn't "bugger all" then it makes sense.  Anna?  I guess it
> > > makes sense if you change it, for spatial memory.
> > 
> > Its not a matter of changing environment variables. You simply expect an
> > application to be consistent among all locales when it comes to the
> > order of fundamental UI elements (besides RTL swap magic of course).
> > 
> > > > > > 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.
> > > I guess its all kinda arbitrary. 
> > 
> > On the order of Drafts and Sent: They seem to be special folders. You
> > can set them arbitrarily according to the prefs dialog. Not sure whether
> > that is useful at all and whether that means that we shouldn't handle
> > them specially here compared to normal folders.
> > On Junk/Trash: I'm not sure which of them is used more frequently. I've
> > swapped them in the attached version. Note that with the current
> > semantics, we can put them in the end without a problem as well. We
> > simply need to change the position members of the struct.
> > 
> > > > > > +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.
> > > I don't know, and yes you may be right, but since the code did that
> > > before, its just better not to change the behaviour.
> > 
> > OK, I've changed that.
> > 
> > > > > > +	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 :).
> > > Well, as a first cut/test, it worked anyway :)
> > 
> > I've revamped the patch and heavily cleaned up the sort function.
> > Besides, it's now extensible through the arrays. Note that the old
> > treeview leaked. Somebody forgot to free the fetched variables.

Index: mail/em-folder-tree-model.c
===================================================================
RCS file: /cvs/gnome/evolution/mail/em-folder-tree-model.c,v
retrieving revision 1.59
diff -u -r1.59 em-folder-tree-model.c
--- mail/em-folder-tree-model.c	29 Jun 2004 10:05:54 -0000	1.59
+++ mail/em-folder-tree-model.c	2 Jul 2004 19:52:34 -0000
@@ -40,6 +40,7 @@
 #include <gal/util/e-xml-utils.h>
 
 #include <camel/camel-file-utils.h>
+#include <camel-vtrash-folder.h>
 
 #include "mail-config.h"
 #include "mail-session.h"
@@ -177,6 +178,64 @@
 			      G_TYPE_STRING);
 }
 
+/* This sets up the order in which treeview elements show up.
+ *   - if type  < 0: Folders
+ *   - if type == 0: Stores
+ *   - if type  > 0: VFolders */
+const static gint
+name_to_position (gchar *name, gint type)
+{
+	const static struct NamePos {
+		gchar *name;
+		gchar *alt_name;
+		gint   position;
+	} *cmp;
+
+	struct NamePos folder_cmp[] = {
+		{ N_("Inbox"),       "INBOX",  -4 },
+		{ N_("Outbox"),      "OUTBOX", -3 },
+		{ N_("Drafts"),      "DRAFTS", -2 },
+		{ N_("Sent"),        "SENT",   -1 },
+		{ CAMEL_VJUNK_NAME,  "JUNK",    1 },
+		{ CAMEL_VTRASH_NAME, "TRASH",   2 },
+		{ NULL,              NULL,      0 }
+	};
+
+	struct NamePos store_cmp[] = {
+		/* Stores */
+		{ N_("On This Computer"), NULL, -1 },
+		{ N_("VFolders"),         NULL,  1 },
+		{ NULL,                   NULL,  0 }
+	};
+
+	struct NamePos vfolder_cmp[] = {
+		/* VFolder Store */
+		{ N_("Unmatched"),   "UNMATCHED", 1 },
+		{ NULL, NULL,                     0 }
+	};
+
+	g_return_val_if_fail (name != NULL, 0);
+
+	if (type < 0)
+		cmp = folder_cmp;
+	else if (type == 0)
+		cmp = store_cmp;
+	else /* type > 0 */
+		cmp = vfolder_cmp;
+
+	int i = 0;
+	while (cmp[i].name != NULL) {
+		if (!strcmp (name, cmp[i].name)
+		    || !strcmp (name, _(cmp[i].name))
+		    || (cmp[i].alt_name != NULL && !strcmp (name, cmp[i].alt_name)))
+			return cmp[i].position;
+
+		i++;
+	}
+
+	return 0;
+}
+
 static int
 sort_cb (GtkTreeModel *model, GtkTreeIter *a, GtkTreeIter *b, gpointer user_data)
 {
@@ -184,50 +243,32 @@
 	char *aname, *bname;
 	CamelStore *store;
 	gboolean is_store;
-	int rv = -2;
+	gint atype, btype, itemtype;
+	gint ret;
 	
 	gtk_tree_model_get (model, a, COL_BOOL_IS_STORE, &is_store,
 			    COL_POINTER_CAMEL_STORE, &store,
 			    COL_STRING_DISPLAY_NAME, &aname, -1);
 	gtk_tree_model_get (model, b, COL_STRING_DISPLAY_NAME, &bname, -1);
-	
-	if (is_store) {
-		/* On This Computer is always first and VFolders is always last */
-		if (!strcmp (aname, _("On This Computer")))
-			rv = -1;
-		else if (!strcmp (bname, _("On This Computer")))
-			rv = 1;
-		else if (!strcmp (aname, _("VFolders")))
-			rv = 1;
-		else if (!strcmp (bname, _("VFolders")))
-			rv = -1;
-	} else if (store == vfolder_store) {
-		/* UNMATCHED is always last */
-		if (aname && !strcmp (aname, _("UNMATCHED")))
-			rv = 1;
-		else if (bname && !strcmp (bname, _("UNMATCHED")))
-			rv = -1;
-	} else {
-		/* Inbox is always first */
-		if (aname && (!strcmp (aname, "INBOX") || !strcmp (aname, _("Inbox"))))
-			rv = -1;
-		else if (bname && (!strcmp (bname, "INBOX") || !strcmp (bname, _("Inbox"))))
-			rv = 1;
-	}
-	
-	if (aname == NULL) {
-		if (bname == NULL)
-			rv = 0;
-	} else if (bname == NULL)
-		rv = 1;
-	
-	if (rv == -2)
-		rv = g_utf8_collate (aname, bname);
-	
+
+	if (!aname && !bname)
+		return 0;
+	else if (!bname)
+		return 1;
+
+	itemtype = is_store ? 0 : store == vfolder_store ? 1 : -1;
+	atype = name_to_position (aname, itemtype);
+	btype = name_to_position (bname, itemtype);
+
+	if (atype != 0 || btype != 0)
+		ret = atype > btype ? 1 : atype < btype ? -1 : 0;
+	else
+		ret = g_utf8_collate (aname, bname);
+
 	g_free (aname);
 	g_free (bname);
-	
-	return rv;
+
+	return ret;
 }
 
 static void