From 24f3024158867f36bc80449423616f6afff4ca55 Mon Sep 17 00:00:00 2001 From: Jani Nikula Date: Tue, 6 Mar 2012 10:20:23 +0200 Subject: [PATCH] Re: [PATCH v2] emacs: Pass a copy to notmuch-saved-search-sort-function --- f6/75c27fe07c425f7897b0343265f9204dea09a4 | 318 ++++++++++++++++++++++ 1 file changed, 318 insertions(+) create mode 100644 f6/75c27fe07c425f7897b0343265f9204dea09a4 diff --git a/f6/75c27fe07c425f7897b0343265f9204dea09a4 b/f6/75c27fe07c425f7897b0343265f9204dea09a4 new file mode 100644 index 000000000..97e79d3f4 --- /dev/null +++ b/f6/75c27fe07c425f7897b0343265f9204dea09a4 @@ -0,0 +1,318 @@ +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-- -- 2.26.2