pull request

pull request

[David Edmondson]

Carl, a couple of patches that I'd like you to consider. They are the
first two on the `dme' branch of git://github.com/dme/notmuch.git.

http://github.com/dme/notmuch/commit/f86254e4509ed02731aa3eaa8541df1f2d11e400
> notmuch-show: Add unix and pretty dates to the JSON output
> 
> Include a 'date_unix' and 'date_pretty' field in the JSON output for
> each message. 'date_pretty' can be used by a UI implementation,
> whereas 'date_unix' is useful when scripting.

http://github.com/dme/notmuch/commit/dfd99e186ffc6b759c4e09a990c43bb6b8743ef2
> notmuch: Add a 'part' subcommand
> 
> A new 'part' subcommand allows the user to extract a single part from a
> MIME message. Usage:
> 
>   notmuch part --part=<n> <search terms>
> 
> The search terms should match only a single message
> (e.g. id:foo@bar.com). The part number specified refers to the part
> identifiers output by `notmuch show'. The content of the part is written
> the stdout with no formatting or identification marks. It is not JSON
> formatted.

The second of these (part) has been the topic of some
discussion. There's a suggestion that a 'cat' subcommand or
'--format=raw' option to the 'show' subcommand would be better. I'm not
particular preference - I just wanted the functionality to use in the
Emacs UI.

(URLs included as the github frontend is quite nice for perusing the
changes.)

dme.
-- 
David Edmondson, http://dme.org

Re: pull request

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 2961 bytes --]

On Thu, 01 Apr 2010 15:41:03 +0100, David Edmondson <dme@dme.org> wrote:
> Carl, a couple of patches that I'd like you to consider. They are the
> first two on the `dme' branch of git://github.com/dme/notmuch.git.

Thanks, David!

I've got your branch fetched here and I plan to start playing with
it. The feature-set of the branch sounds extremely compelling, so I'm
looking forward to usability improvements from this. Thanks so much.

> http://github.com/dme/notmuch/commit/f86254e4509ed02731aa3eaa8541df1f2d11e400
> > notmuch-show: Add unix and pretty dates to the JSON output
> > 
> > Include a 'date_unix' and 'date_pretty' field in the JSON output for
> > each message. 'date_pretty' can be used by a UI implementation,
> > whereas 'date_unix' is useful when scripting.

With "date_unix" it's easy enough to guess what the value is. But
"date_pretty" is much more ambiguous. I assumed that this would be an
RFC 822 date string, (but then found that it's the relative date that
"notmuch show" is using currently).

I think I'd rather see that named "date_relative", (or dropped with the
expectation that callers can decide how to format dates on their own).

> > A new 'part' subcommand allows the user to extract a single part from a
> > MIME message. Usage:
> > 
> >   notmuch part --part=<n> <search terms>

This sounds like great functionality, and is something I had intended to
do very early after starting notmuch.el but never got around to.

> > The search terms should match only a single message
> > (e.g. id:foo@bar.com). The part number specified refers to the part
> > identifiers output by `notmuch show'. The content of the part is written
> > the stdout with no formatting or identification marks. It is not JSON
> > formatted.

The above documentation isn't quite complete to me. Is MIME decoding
handled by this or not? Also, the documentation says the search terms
"should match" only one message, but what does the implementation do if
more than one message is matched? It would be good to document that as
well.

More significantly, this level of documentation needs to be put into the
"notmuch help" output (instead of the placeholder that's there in the
current patch). It also needs to be added to the man page in notmuch.1.

> The second of these (part) has been the topic of some
> discussion. There's a suggestion that a 'cat' subcommand or
> '--format=raw' option to the 'show' subcommand would be better. I'm not
> particular preference - I just wanted the functionality to use in the
> Emacs UI.

One other approach that I imagined with the json output would be to
simply include all of the MIME parts of all messages directly in the
json-format output from "notmuch show". Does json have any particular
way of encodign a binary blob? If not, should we just have one single
encoding here? (Such as BASE64 within a json string?)

Looking forward to more,

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: pull request

[David Bremner]

On Thu, 01 Apr 2010 14:09:57 -0700, Carl Worth <cworth@cworth.org> wrote:

> More significantly, this level of documentation needs to be put into the
> "notmuch help" output (instead of the placeholder that's there in the
> current patch). It also needs to be added to the man page in notmuch.1.

Allow me to shamelessly plug my podification of notmuch docs :)

  id:1262281169-24909-1-git-send-email-david@tethera.net

This would allow only maintaining the docs in one place. 

David

Re: pull request

[David Edmondson]

On Thu, 01 Apr 2010 14:09:57 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Thu, 01 Apr 2010 15:41:03 +0100, David Edmondson <dme@dme.org> wrote:
> > Carl, a couple of patches that I'd like you to consider. They are the
> > first two on the `dme' branch of git://github.com/dme/notmuch.git.

The updated changes are on the 'dme-for-cworth' branch now.

> > http://github.com/dme/notmuch/commit/f86254e4509ed02731aa3eaa8541df1f2d11e400
> > > notmuch-show: Add unix and pretty dates to the JSON output
> > > 
> > > Include a 'date_unix' and 'date_pretty' field in the JSON output for
> > > each message. 'date_pretty' can be used by a UI implementation,
> > > whereas 'date_unix' is useful when scripting.
> 
> With "date_unix" it's easy enough to guess what the value is. But
> "date_pretty" is much more ambiguous. I assumed that this would be an
> RFC 822 date string, (but then found that it's the relative date that
> "notmuch show" is using currently).
> 
> I think I'd rather see that named "date_relative", (or dropped with the
> expectation that callers can decide how to format dates on their own).

I renamed it to 'date_relative'.

Keeping the creation of the relative date strings in one place struck me
as a good idea - there will consistency between the two places it is
used (search mode and show mode) and if the `notmuch' command is
localised the Emacs UI will immediately benefit.

> > > The search terms should match only a single message
> > > (e.g. id:foo@bar.com). The part number specified refers to the part
> > > identifiers output by `notmuch show'. The content of the part is written
> > > the stdout with no formatting or identification marks. It is not JSON
> > > formatted.
> 
> The above documentation isn't quite complete to me. Is MIME decoding
> handled by this or not? Also, the documentation says the search terms
> "should match" only one message, but what does the implementation do if
> more than one message is matched? It would be good to document that as
> well.
> 
> More significantly, this level of documentation needs to be put into the
> "notmuch help" output (instead of the placeholder that's there in the
> current patch). It also needs to be added to the man page in
> notmuch.1.

The documentation is updated. Sorry for being lazy.

> > The second of these (part) has been the topic of some
> > discussion. There's a suggestion that a 'cat' subcommand or
> > '--format=raw' option to the 'show' subcommand would be better. I'm not
> > particular preference - I just wanted the functionality to use in the
> > Emacs UI.
> 
> One other approach that I imagined with the json output would be to
> simply include all of the MIME parts of all messages directly in the
> json-format output from "notmuch show". Does json have any particular
> way of encodign a binary blob? If not, should we just have one single
> encoding here? (Such as BASE64 within a json string?)

I'm sure that JSON could express the blob. There were two reasons that I
decided not to include the full message in the JSON output:

        - some of the parts can be very large, causing Emacs to spend
          considerably time loading the part (and consume a bunch of
          memory). This would be particularly noticeable in a thread
          where many of the messages include large parts - the UI will
          load all of the parts, even if you've already seen the
          message.

        - there are some parts that the UI will probably never display
          inline usefully (OpenOffice?). For those parts it's quite
          wasteful to have the UI pull them in.

There's obviously a compromise to be had. If we agreed to include
text/html parts in the JSON output it would make sense to me - maybe all
text/* parts should be there. There are probably others that would be
useful.

The later 'display all parts' version of notmuch-show is able to use
either inline or external content to display parts (it uses that
included in the JSON output if present).

dme.
-- 
David Edmondson, http://dme.org

Re: pull request

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]

On Fri, 02 Apr 2010 09:53:04 +0100, David Edmondson <dme@dme.org> wrote:
> The updated changes are on the 'dme-for-cworth' branch now.

Thanks for the quick update, David!

I've pushed this now.

Having played with it a tiny bit, I do think that since the part command
always requires a --part option that we might as well just make it part
of notmuch show. That is:

	notmuch show --part=2 id:foo

instead of:

	notmuch part --part=2 id:foo

It's no more to type and it keeps our top-level command-set simpler.

(Along that same line, I think I'd like to fold "notmuch search-tags"
and perhaps "notmuch count" into sub-options of "notmuch search" as
well.)

But I didn't make the patch block on that. I'm asserting that the
notmuch command-line interface is not yet set in stone and that user
should expect changes.

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

pull request [was Re: pull request]

[David Edmondson]

On Fri, 02 Apr 2010 15:53:34 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Fri, 02 Apr 2010 09:53:04 +0100, David Edmondson <dme@dme.org> wrote:
> > The updated changes are on the 'dme-for-cworth' branch now.
> 
> Thanks for the quick update, David!
> 
> I've pushed this now.

Thanks. Here's another set to consider. They are in the 'dme' branch.

* commit 94893f25d36aaf43487e111fbfba4f7dae808dd2
| Author: David Edmondson <dme@dme.org>
| Date:   Tue Mar 23 07:04:34 2010 +0000
| 
|     emacs/notmuch.el: Improve tag highlighting in search mode
|     
|     Assume that tags never include an opening bracket, and hence improve
|     the regular expression used to highlight them. This avoids false
|     matches where the 'from' address of a thread participant includes an
|     opening bracket.

* commit 4de9f3f09e998d7312be2a1c08526e59bbf135a9
| Author: David Edmondson <dme@dme.org>
| Date:   Sun Mar 21 09:54:08 2010 +0000
| 
|     emacs/Makefile.local: Use makefile mode

* commit f7ecad654fd8d0274fc75833d92117c8e496bcea
| Author: David Edmondson <dme@dme.org>
| Date:   Thu Apr 1 18:36:21 2010 +0100
| 
|     emacs: Move notmuch-show functionality to notmuch-show.el
|     
|     To ease the transition to a JSON based implementation of
|     `notmuch-show', move the current implementation into a separate file.

* commit a9590dfb4efc2c05a35948ef4522c362eb788c10
| Author: David Edmondson <dme@dme.org>
| Date:   Thu Apr 1 11:38:30 2010 +0100
| 
|     Makefile: Add the emacs directory to load-path when compiling

dme.
-- 
David Edmondson, http://dme.org

Re: pull request

[David Edmondson]

On Fri, 02 Apr 2010 15:53:34 -0700, Carl Worth <cworth@cworth.org> wrote:
> Having played with it a tiny bit, I do think that since the part command
> always requires a --part option that we might as well just make it part
> of notmuch show. That is:
> 
> 	notmuch show --part=2 id:foo
> 
> instead of:
> 
> 	notmuch part --part=2 id:foo
> 
> It's no more to type and it keeps our top-level command-set simpler.

As part of having more information about MIME structure in the JSON
output (multipart/alternative, for example), I think that we (I, if I
get to it first) will need to re-work the `show' implementation
somewhat. We should wait until doing that work to merge the `--part'
support across.

dme.
-- 
David Edmondson, http://dme.org

Re: pull request [was Re: pull request]

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 3718 bytes --]

On Sat, 03 Apr 2010 07:32:44 +0100, David Edmondson <dme@dme.org> wrote:
> * commit a9590dfb4efc2c05a35948ef4522c362eb788c10
> | Author: David Edmondson <dme@dme.org>
> | Date:   Thu Apr 1 11:38:30 2010 +0100
> | 
> |     Makefile: Add the emacs directory to load-path when compiling

That's a nice one-line summary of the commit that says "what" the patch
does just fine. But the commit is missing the rest of the commit message
that should give the "why".

What's the motivation of the change? Is something perhaps broken without
this? Or is this a preparation for something else that will be coming
along in a future commit?

From looking at the next commit, I assume this is to enable the
"(require 'notmuch-show)" that is being added subsequently. So I've just
noted that.

> * commit 4de9f3f09e998d7312be2a1c08526e59bbf135a9
> | Author: David Edmondson <dme@dme.org>
> | Date:   Sun Mar 21 09:54:08 2010 +0000
> | 
> |     emacs/Makefile.local: Use makefile mode

I added similar treatment to the other instances of files named
Makefile.local.

> * commit 94893f25d36aaf43487e111fbfba4f7dae808dd2
> | Author: David Edmondson <dme@dme.org>
> | Date:   Tue Mar 23 07:04:34 2010 +0000
> | 
> |     emacs/notmuch.el: Improve tag highlighting in search mode
> |     
> |     Assume that tags never include an opening bracket, and hence improve
> |     the regular expression used to highlight them. This avoids false
> |     matches where the 'from' address of a thread participant includes an
> |     opening bracket.

Thanks. That's a good fix.

The above are all pushed.

> * commit f7ecad654fd8d0274fc75833d92117c8e496bcea
> | Author: David Edmondson <dme@dme.org>
> | Date:   Thu Apr 1 18:36:21 2010 +0100
> | 
> |     emacs: Move notmuch-show functionality to notmuch-show.el
> |     
> |     To ease the transition to a JSON based implementation of
> |     `notmuch-show', move the current implementation into a separate file.

This is definitely a nice improvement in modularization. But there are
some aspects of doing multiple-file emacs code that I'm unclear on.

If I apply this patch as is, then when compiling the notmuch-show.el I
get the following warnings:

  In notmuch-show:
  notmuch-show.el:969:34:Warning: reference to free variable `notmuch-command'

  In end of data:
  notmuch-show.el:983:1:Warning: the following functions are not known to be
      defined: point-invisible-p, mail-header-extract-no-properties,
      notmuch-select-tag-with-completion, union, intersection, set-difference,
      notmuch-search-show-thread, mm-display-parts, mm-dissect-buffer,
      notmuch-save-attachments, notmuch-count-attachments, notmuch-reply,
      mm-handle-type, mm-display-part, notmuch-fontify-headers

I can eliminate a few of these by copying the various require calls from
notmuch.el to notmuch-show.el, but that still leaves problems for all of
the functionality defined in notmuch.el and referenced in
notmuch-show.el:

  In notmuch-show:
  notmuch-show.el:973:34:Warning: reference to free variable `notmuch-command'

  In end of data:
  notmuch-show.el:987:1:Warning: the following functions are not known to be
      defined: point-invisible-p, notmuch-select-tag-with-completion,
      notmuch-search-show-thread, notmuch-save-attachments,
      notmuch-count-attachments, notmuch-reply, notmuch-fontify-headers

Does anyone know the right way to fix this? I'd like to get the output
clean as I plan to move the compilation of the emacs code from "make
install-emacs" to "make", (made conditional on a check for the presence
of emacs by configure).

So I haven't merged this commit yet.

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: pull request

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

On Sat, 03 Apr 2010 07:34:57 +0100, David Edmondson <dme@dme.org> wrote:
> As part of having more information about MIME structure in the JSON
> output (multipart/alternative, for example), I think that we (I, if I
> get to it first) will need to re-work the `show' implementation
> somewhat. We should wait until doing that work to merge the `--part'
> support across.

OK. I've made a note about that in TODO now at least.

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: pull request [was Re: pull request]

[David Bremner]

On Sat, 03 Apr 2010 12:37:46 -0700, Carl Worth <cworth@cworth.org> wrote:
> 
> If I apply this patch as is, then when compiling the notmuch-show.el I
> get the following warnings:
> 
>   In notmuch-show:
>   notmuch-show.el:969:34:Warning: reference to free variable `notmuch-command'
> 
>   In end of data:
>   notmuch-show.el:983:1:Warning: the following functions are not known to be
>       defined: point-invisible-p, mail-header-extract-no-properties,
>       notmuch-select-tag-with-completion, union, intersection, set-difference,
>       notmuch-search-show-thread, mm-display-parts, mm-dissect-buffer,
>       notmuch-save-attachments, notmuch-count-attachments, notmuch-reply,
>       mm-handle-type, mm-display-part, notmuch-fontify-headers
> 

This is my understanding from staring at the code for a few other
packages; any experts feel free to contradict.

The warnings about unknown functions can be eliminated by use the
declare-function macro; if you have emacs lisp reference manual (it
required the package emacs23-common-non-dfsg on Debian) then you can
run:

ESC ESC : (info "(elisp)Declaring Functions") <RETURN>

For variables, the obvious approach is to make a file 'notmuch-vars.el'
and put the variables we need in several files there.

Re: pull request [was Re: pull request]

[David Edmondson]

On Sat, 03 Apr 2010 18:21:57 -0300, David Bremner <bremner@unb.ca> wrote:
> The warnings about unknown functions can be eliminated by use the
> declare-function macro; if you have emacs lisp reference manual (it
> required the package emacs23-common-non-dfsg on Debian) then you can
> run:
> 
> ESC ESC : (info "(elisp)Declaring Functions") <RETURN>
> 
> For variables, the obvious approach is to make a file 'notmuch-vars.el'
> and put the variables we need in several files there.

I'll declare common variables and 'exported' functions in
'notmuch-lib.el'. It may take a few days (I'm supposed to be on
holiday).

Carl: is a single changeset that creates notmuch-lib.el and moves the
show implementation into notmuch-show.el okay?

dme.
-- 
David Edmondson, http://dme.org

Re: pull request [was Re: pull request]

[David Edmondson]

On Sun, 04 Apr 2010 08:33:00 +0100, David Edmondson <dme@dme.org> wrote:
> I'll declare common variables and 'exported' functions in
> 'notmuch-lib.el'. It may take a few days (I'm supposed to be on
> holiday).

It was quicker than expected. Using 'declare-function' in a file that is
required didn't have the right effect (it seemed to be ignored), so the
'declare-function' calls are directly in `notmuch-show.el'. I get no
warnings when compiling now.

It's the 'for-cworth' branch of git://github.com/dme/notmuch.git.

There's also a simple update to tell git to ignore notmuch-shared.

dme.
-- 
David Edmondson, http://dme.org

Re: pull request [was Re: pull request]

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 253 bytes --]

On Sun, 04 Apr 2010 09:08:34 +0100, David Edmondson <dme@dme.org> wrote:
> It's the 'for-cworth' branch of git://github.com/dme/notmuch.git.
> 
> There's also a simple update to tell git to ignore notmuch-shared.

Thanks. This is pushed.

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]