Return-Path: X-Original-To: notmuch@notmuchmail.org Delivered-To: notmuch@notmuchmail.org Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 874BA431FAF for ; Tue, 6 Mar 2012 00:20:29 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.699 X-Spam-Level: X-Spam-Status: No, score=-0.699 tagged_above=-999 required=5 tests=[HTML_MESSAGE=0.001, RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 2w-l4cFUj+9W for ; Tue, 6 Mar 2012 00:20:25 -0800 (PST) Received: from mail-pz0-f45.google.com (mail-pz0-f45.google.com [209.85.210.45]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (No client certificate requested) by olra.theworths.org (Postfix) with ESMTPS id 79689431FAE for ; Tue, 6 Mar 2012 00:20:25 -0800 (PST) Received: by dadp14 with SMTP id p14so4753431dad.18 for ; Tue, 06 Mar 2012 00:20:23 -0800 (PST) Received-SPF: pass (google.com: domain of jani@nikula.org designates 10.68.239.195 as permitted sender) client-ip=10.68.239.195; Authentication-Results: mr.google.com; spf=pass (google.com: domain of jani@nikula.org designates 10.68.239.195 as permitted sender) smtp.mail=jani@nikula.org Received: from mr.google.com ([10.68.239.195]) by 10.68.239.195 with SMTP id vu3mr29119326pbc.49.1331022023585 (num_hops = 1); Tue, 06 Mar 2012 00:20:23 -0800 (PST) MIME-Version: 1.0 Received: by 10.68.239.195 with SMTP id vu3mr25105581pbc.49.1331022023409; Tue, 06 Mar 2012 00:20:23 -0800 (PST) Received: by 10.68.12.103 with HTTP; Tue, 6 Mar 2012 00:20:23 -0800 (PST) Received: by 10.68.12.103 with HTTP; Tue, 6 Mar 2012 00:20:23 -0800 (PST) In-Reply-To: <87zkbukb59.fsf@gmail.com> References: <1330613059-5130-1-git-send-email-daniel@schoepe.org> <1330613059-5130-2-git-send-email-daniel@schoepe.org> <87zkbukb59.fsf@gmail.com> Date: Tue, 6 Mar 2012 10:20:23 +0200 Message-ID: Subject: Re: [PATCH v2] emacs: Pass a copy to notmuch-saved-search-sort-function From: Jani Nikula To: Dmitry Kurochkin Content-Type: multipart/alternative; boundary=047d7b33d5cce4bfa104ba8eb662 X-Gm-Message-State: ALoCoQkkZWmRJUzPGJ8IzVvJjStyYZMwy7rS9BfK1ngtNpK3IOwCF2KBhsmx6irk5rzMTkA9fbvg Cc: notmuch@notmuchmail.org X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Mar 2012 08:20:29 -0000 --047d7b33d5cce4bfa104ba8eb662 Content-Type: text/plain; charset=UTF-8 On Mar 5, 2012 11:11 PM, "Dmitry Kurochkin" wrote: > > On Mon, 5 Mar 2012 22:55:54 +0200, Jani Nikula wrote: > > On Mar 5, 2012 5:43 PM, "Dmitry Kurochkin" > > wrote: > > > > > > On Mon, 05 Mar 2012 12:17:43 +0100, Daniel Schoepe > > wrote: > > > > On Mon, 05 Mar 2012 06:21:52 +0400, Dmitry Kurochkin < > > dmitry.kurochkin@gmail.com> wrote: > > > > > On Thu, 1 Mar 2012 21:24:38 +0100, Daniel Schoepe < daniel@schoepe.org> > > wrote: > > > > > > notmuch-saved-search-sort-function might destructively modify its > > > > > > input (`sort' does that, for instance), so it should not be given > > > > > > notmuch-saved-searches directly. > > > > > > --- > > > > > > > > > > -1 > > > > > > > > > > I think we should require `notmuch-saved-search-sort-function' not to > > > > > have side effects. Current documentation should be more clear about > > > > > this. We need to fix `notmuch-sort-saved-searches' to copy the list > > > > > before calling `sort'. But we should not do it in > > > > > `notmuch-hello-insert-saved-searches' for any sorting function (which > > > > > may not need this copying). > > > > > > > > My reasoning was that since sort is such a common function, many users > > > > will probably use sort for their own sorting functions, not realizing > > > > that it has side effects. This will lead to confusing behavior that's > > > > not so easy to track down. > > > > > > > > Copying the list of saved searches when running notmuch-hello does not > > > > seem be relevant to performance to me, since it's a) not called that > > > > often and b) the list of saved searches will rarely exceed 30 elements. > > > > > > > > Hence, this way we can avoid some headaches for users who define their > > > > own sorting functions at a negligible (performance) cost. Incidentally, > > > > this is also how notmuch-hello did it before the user-defined sections > > > > patches. > > > > > > > > > > I do not buy the argument that we should help users who implement their > > > own sorting functions but do not read documentation for functions they > > > use. Apparently, those who implemented the `sort' function had similar > > > ideas. And I do not think it is our job to add workarounds for it. > > > > > > An alternative (and IMO better) solution would be to allow customization > > > of compare function used for sorting instead of the sorting function > > > itself. > > > > Providing the customization of the sort function is more powerful than the > > compare function. In the case of saved searches I can imagine people might > > want to partially use the original order while sort the rest (e.g. > > important ones first in predefined order, others sorted). > > Valid point. > > > In fact this also > > allows dropping out some elements. And renaming. And changing the queries... > > > > (I had something like that in mind originally but then settled with just > > capitalizing the important ones to show them first.) > > > > All of these are invalid usages of `notmuch-saved-search-sort-function'. > The function is meant for sorting only (hence the name). So the code > might assume that the function does only sorting. > > I do not understand why we need such functionality (renaming, > capitalizing, etc.). You can just rename the query itself if you want > to. Should be easier IMO. Just for the record, I have a few important searches capitalized in the saved searches and just use the regular sort. Capitalized entries sort before the lowercase ones. > But if we need such functionality, we should > not misuse sorting function for it. We can add `notmuch-saved-searches' > function which would return saved searches list (sorted, renamed and > mangled in any other way). By default it would return > `notmuch-saved-searches' variable as is. Agreed. As to the problem at hand, we should just fix the sort function not to modify its input. I wouldn't hold my breath waiting for someone to provide patches for the rest... BR, Jani. --047d7b33d5cce4bfa104ba8eb662 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Mar 5, 2012 11:11 PM, "Dmitry Kurochkin" <dmitry.kurochkin@gmail.com> wrote:
>
> On Mon, 5 Mar 2012 22:55:54 +0200, Jani Nikula <jani@nikula.org> wrote:
> > On Mar 5, 2012 5:43 PM, "Dmitry Kurochkin" <dmitry.kurochkin@gmail.com> > > wrote:
> > >
> > > On Mon, 05 Mar 2012 12:17:43 +0100, Daniel Schoepe <daniel@schoepe.org>
> > wrote:
> > > > On Mon, 05 Mar 2012 06:21:52 +0400, Dmitry Kurochkin &l= t;
> > dmitry.kurochkin@gm= ail.com> wrote:
> > > > > On Thu, =C2=A01 Mar 2012 21:24:38 +0100, Daniel Sc= hoepe <daniel@schoepe.org><= br> > > wrote:
> > > > > > notmuch-saved-search-sort-function might dest= ructively modify its
> > > > > > input (`sort' does that, for instance), s= o it should not be given
> > > > > > notmuch-saved-searches directly.
> > > > > > ---
> > > > >
> > > > > -1
> > > > >
> > > > > I think we should require `notmuch-saved-search-so= rt-function' not to
> > > > > have side effects. =C2=A0Current documentation sho= uld be more clear about
> > > > > this. =C2=A0We need to fix `notmuch-sort-saved-sea= rches' to copy the list
> > > > > before calling `sort'. =C2=A0But we should not= do it in
> > > > > `notmuch-hello-insert-saved-searches' for any = sorting function (which
> > > > > may not need this copying).
> > > >
> > > > My reasoning was that since sort is such a common funct= ion, many users
> > > > will probably use sort for their own sorting functions,= not realizing
> > > > that it has side effects. This will lead to confusing b= ehavior that's
> > > > not so easy to track down.
> > > >
> > > > Copying the list of saved searches when running notmuch= -hello does not
> > > > seem be relevant to performance to me, since it's a= ) not called that
> > > > often and b) the list of saved searches will rarely exc= eed 30 elements.
> > > >
> > > > Hence, this way we can avoid some headaches for users w= ho define their
> > > > own sorting functions at a negligible (performance) cos= t. Incidentally,
> > > > this is also how notmuch-hello did it before the user-d= efined sections
> > > > patches.
> > > >
> > >
> > > I do not buy the argument that we should help users who impl= ement their
> > > own sorting functions but do not read documentation for func= tions they
> > > use. =C2=A0Apparently, those who implemented the `sort' = function had similar
> > > ideas. =C2=A0And I do not think it is our job to add workaro= unds for it.
> > >
> > > An alternative (and IMO better) solution would be to allow c= ustomization
> > > of compare function used for sorting instead of the sorting = function
> > > itself.
> >
> > Providing the customization of the sort function is more powerful= than the
> > compare function. In the case of saved searches I can imagine peo= ple might
> > want to partially use the original order while sort the rest (e.g= .
> > important ones first in predefined order, others sorted).
>
> Valid point.
>
> > In fact this also
> > allows dropping out some elements. And renaming. And changing the= queries...
> >
> > (I had something like that in mind originally but then settled wi= th just
> > capitalizing the important ones to show them first.)
> >
>
> All of these are invalid usages of `notmuch-saved-search-sort-function= '.
> The function is meant for sorting only (hence the name). =C2=A0So the = code
> might assume that the function does only sorting.
>
> I do not understand why we need such functionality (renaming,
> capitalizing, etc.). =C2=A0You can just rename the query itself if you= want
> to. =C2=A0Should be easier IMO. =C2=A0

Just for the record, I have a few important searches capitalized in the = saved searches and just use the regular sort. Capitalized entries sort befo= re the lowercase ones.

> But if we need such functionality, we should
> not misuse sorting function for it. =C2=A0We can add `notmuch-saved-se= arches'
> function which would return saved searches list (sorted, renamed and > mangled in any other way). =C2=A0By default it would return
> `notmuch-saved-searches' variable as is.

Agreed.

As to the problem at hand, we should just fix the sort function not to m= odify its input. I wouldn't hold my breath waiting for someone to provi= de patches for the rest...

BR,
Jani.

--047d7b33d5cce4bfa104ba8eb662--