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

[Christian Neumair <chris gnome-de org>]

Am Freitag, den 02.07.2004, 21:53 +0200 schrieb Christian Neumair:
> 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.

Anybody willing to review it?

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.