[PATCH] emacs: Improve `notmuch-hello' display on ttys.

[PATCH] emacs: Improve `notmuch-hello' display on ttys.

[David Edmondson]

Inserting spaces to pad out columns is good, except when the padding
makes the line wider than the window. This looks particularly bad on a
tty where there is no fringe.

Hence, avoid padding the last column on each row.
---

Thanks to j4ni in #notmuch for spotting this.

 emacs/notmuch-hello.el |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 333d4c1..02017ce 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -299,15 +299,17 @@ should be. Returns a cons cell `(tags-per-line width)'."
 			       :notify #'notmuch-hello-widget-search
 			       :notmuch-search-terms query
 			       formatted-name)
-		;; Insert enough space to consume the rest of the
-		;; column.  Because the button for the name is `(1+
-		;; (length name))' long (due to the trailing space) we
-		;; can just insert `(- widest (length name))' spaces -
-		;; the column separator is included in the button if
-		;; `(equal widest (length name)'.
-		(widget-insert (make-string (max 1
-						 (- widest (length name)))
-					    ? ))))
+		(unless (eq (% count tags-per-line) (1- tags-per-line))
+		  ;; If this is not the last tag on the line, insert
+		  ;; enough space to consume the rest of the column.
+		  ;; Because the button for the name is `(1+ (length
+		  ;; name))' long (due to the trailing space) we can
+		  ;; just insert `(- widest (length name))' spaces - the
+		  ;; column separator is included in the button if
+		  ;; `(equal widest (length name)'.
+		  (widget-insert (make-string (max 1
+						   (- widest (length name)))
+					      ? )))))
 	    (setq count (1+ count))
 	    (if (eq (% count tags-per-line) 0)
 		(widget-insert "\n")))
-- 
1.7.7.3

Re: [PATCH] emacs: Improve `notmuch-hello' display on ttys.

[Austin Clements]

LGTM, though would it be easier to put this in the else clause of the
if after the setq count?

Is it possible for a tag in the last column to be just long enough to
make the line still wrap?  Somehow my current tag set doesn't trigger
this bug, so I can't test this case (and I admit I can't follow
notmuch-hello-insert-tags well enough to reason this out).

Quoth David Edmondson on Jan 10 at 10:15 am:
> Inserting spaces to pad out columns is good, except when the padding
> makes the line wider than the window. This looks particularly bad on a
> tty where there is no fringe.
> 
> Hence, avoid padding the last column on each row.
> ---
> 
> Thanks to j4ni in #notmuch for spotting this.
> 
>  emacs/notmuch-hello.el |   20 +++++++++++---------
>  1 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index 333d4c1..02017ce 100644
> --- a/emacs/notmuch-hello.el
> +++ b/emacs/notmuch-hello.el
> @@ -299,15 +299,17 @@ should be. Returns a cons cell `(tags-per-line width)'."
>  			       :notify #'notmuch-hello-widget-search
>  			       :notmuch-search-terms query
>  			       formatted-name)
> -		;; Insert enough space to consume the rest of the
> -		;; column.  Because the button for the name is `(1+
> -		;; (length name))' long (due to the trailing space) we
> -		;; can just insert `(- widest (length name))' spaces -
> -		;; the column separator is included in the button if
> -		;; `(equal widest (length name)'.
> -		(widget-insert (make-string (max 1
> -						 (- widest (length name)))
> -					    ? ))))
> +		(unless (eq (% count tags-per-line) (1- tags-per-line))
> +		  ;; If this is not the last tag on the line, insert
> +		  ;; enough space to consume the rest of the column.
> +		  ;; Because the button for the name is `(1+ (length
> +		  ;; name))' long (due to the trailing space) we can
> +		  ;; just insert `(- widest (length name))' spaces - the
> +		  ;; column separator is included in the button if
> +		  ;; `(equal widest (length name)'.
> +		  (widget-insert (make-string (max 1
> +						   (- widest (length name)))
> +					      ? )))))
>  	    (setq count (1+ count))
>  	    (if (eq (% count tags-per-line) 0)
>  		(widget-insert "\n")))

Re: [PATCH] emacs: Improve `notmuch-hello' display on ttys.

[David Edmondson]

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

On Tue, 10 Jan 2012 10:36:50 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> LGTM, though would it be easier to put this in the else clause of the
> if after the setq count?

Agreed. I got confused thinking about it due to the empty elements in
the matrix, but perhaps that doesn't matter. I'll continue to think, but
would rather not delay fixing the bug.

> Is it possible for a tag in the last column to be just long enough to
> make the line still wrap?  Somehow my current tag set doesn't trigger
> this bug, so I can't test this case (and I admit I can't follow
> notmuch-hello-insert-tags well enough to reason this out).

With a sufficiently narrow window it's always possible to generate wrap,
of course. I couldn't make it happen for any window width that seemed
reasonable.

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

Re: [PATCH] emacs: Improve `notmuch-hello' display on ttys.

[Austin Clements]

Quoth David Edmondson on Jan 10 at  3:47 pm:
> On Tue, 10 Jan 2012 10:36:50 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > LGTM, though would it be easier to put this in the else clause of the
> > if after the setq count?
> 
> Agreed. I got confused thinking about it due to the empty elements in
> the matrix, but perhaps that doesn't matter. I'll continue to think, but
> would rather not delay fixing the bug.

Fair enough.

> > Is it possible for a tag in the last column to be just long enough to
> > make the line still wrap?  Somehow my current tag set doesn't trigger
> > this bug, so I can't test this case (and I admit I can't follow
> > notmuch-hello-insert-tags well enough to reason this out).
> 
> With a sufficiently narrow window it's always possible to generate wrap,
> of course. I couldn't make it happen for any window width that seemed
> reasonable.

I should have specified more than one column.  Clearly if you're down
to one column it's always possible to wrap, but if you have more than
one, does the code always reduce the number of columns in preference
to allowing a particularly long tag name to wrap a line?  Based on
your testing, it sounds like it handles this correctly.  (Even if it
didn't, this shouldn't block this patch; it's related, but not the
same issue.)

Re: [PATCH] emacs: Improve `notmuch-hello' display on ttys.

[David Edmondson]

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

On Tue, 10 Jan 2012 11:05:02 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > > Is it possible for a tag in the last column to be just long enough to
> > > make the line still wrap?  Somehow my current tag set doesn't trigger
> > > this bug, so I can't test this case (and I admit I can't follow
> > > notmuch-hello-insert-tags well enough to reason this out).
> > 
> > With a sufficiently narrow window it's always possible to generate wrap,
> > of course. I couldn't make it happen for any window width that seemed
> > reasonable.
> 
> I should have specified more than one column.  Clearly if you're down
> to one column it's always possible to wrap, but if you have more than
> one, does the code always reduce the number of columns in preference
> to allowing a particularly long tag name to wrap a line?

It's supposed to do that.

I have a vague recollection that someone reported a bug where
`(window-width)' was not the right value to use if `line-number-mode' is
enabled, but I can't find it again now. (Someone should build an email
client that allows easy searching.)

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

Re: [PATCH] emacs: Improve `notmuch-hello' display on ttys.

[Jani Nikula]

On Tue, 10 Jan 2012 10:15:28 +0000, David Edmondson <dme@dme.org> wrote:
> Inserting spaces to pad out columns is good, except when the padding
> makes the line wider than the window. This looks particularly bad on a
> tty where there is no fringe.
> 
> Hence, avoid padding the last column on each row.
> ---
> 
> Thanks to j4ni in #notmuch for spotting this.

Thanks for fixing this. This patch works for me.

BR,
j4ni.


> 
>  emacs/notmuch-hello.el |   20 +++++++++++---------
>  1 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index 333d4c1..02017ce 100644
> --- a/emacs/notmuch-hello.el
> +++ b/emacs/notmuch-hello.el
> @@ -299,15 +299,17 @@ should be. Returns a cons cell `(tags-per-line width)'."
>  			       :notify #'notmuch-hello-widget-search
>  			       :notmuch-search-terms query
>  			       formatted-name)
> -		;; Insert enough space to consume the rest of the
> -		;; column.  Because the button for the name is `(1+
> -		;; (length name))' long (due to the trailing space) we
> -		;; can just insert `(- widest (length name))' spaces -
> -		;; the column separator is included in the button if
> -		;; `(equal widest (length name)'.
> -		(widget-insert (make-string (max 1
> -						 (- widest (length name)))
> -					    ? ))))
> +		(unless (eq (% count tags-per-line) (1- tags-per-line))
> +		  ;; If this is not the last tag on the line, insert
> +		  ;; enough space to consume the rest of the column.
> +		  ;; Because the button for the name is `(1+ (length
> +		  ;; name))' long (due to the trailing space) we can
> +		  ;; just insert `(- widest (length name))' spaces - the
> +		  ;; column separator is included in the button if
> +		  ;; `(equal widest (length name)'.
> +		  (widget-insert (make-string (max 1
> +						   (- widest (length name)))
> +					      ? )))))
>  	    (setq count (1+ count))
>  	    (if (eq (% count tags-per-line) 0)
>  		(widget-insert "\n")))
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: Improve `notmuch-hello' display on ttys.

[Jani Nikula]

On Tue, 10 Jan 2012 22:41:49 +0200, Jani Nikula <jani@nikula.org> wrote:
> On Tue, 10 Jan 2012 16:18:21 +0000, David Edmondson <dme@dme.org> wrote:
> > I have a vague recollection that someone reported a bug where
> > `(window-width)' was not the right value to use if `line-number-mode' is
> > enabled, but I can't find it again now. (Someone should build an email
> > client that allows easy searching.)
> 
> Do you perhaps mean this: id:"87k47nlpzb.fsf@nausicaa.localdomain"

[Now also to the list. Got bitten by my own patch binding 'r' to
reply-to-sender. Perhaps it's really too drastic to change that now...]

Re: [PATCH] emacs: Improve `notmuch-hello' display on ttys.

[David Edmondson]

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

On Tue, 10 Jan 2012 22:45:47 +0200, Jani Nikula <jani@nikula.org> wrote:
> On Tue, 10 Jan 2012 22:41:49 +0200, Jani Nikula <jani@nikula.org> wrote:
> > On Tue, 10 Jan 2012 16:18:21 +0000, David Edmondson <dme@dme.org> wrote:
> > > I have a vague recollection that someone reported a bug where
> > > `(window-width)' was not the right value to use if `line-number-mode' is
> > > enabled, but I can't find it again now. (Someone should build an email
> > > client that allows easy searching.)
> > 
> > Do you perhaps mean this: id:"87k47nlpzb.fsf@nausicaa.localdomain"
> 
> [Now also to the list. Got bitten by my own patch binding 'r' to
> reply-to-sender. Perhaps it's really too drastic to change that now...]

Yes, that one.

Ivy: Are you likely to add a test and re-submit the patch?

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

Re: [PATCH] emacs: Improve `notmuch-hello' display on ttys.

[Ivy Foster]

On 11 Jan 2012, at  8:27 am +0000, David Edmondson wrote:
> On Tue, 10 Jan 2012 22:45:47 +0200, Jani Nikula <jani@nikula.org> wrote:
> > On Tue, 10 Jan 2012 22:41:49 +0200, Jani Nikula <jani@nikula.org> wrote:
> > > On Tue, 10 Jan 2012 16:18:21 +0000, David Edmondson <dme@dme.org> wrote:
> > > > I have a vague recollection that someone reported a bug where
> > > > `(window-width)' was not the right value to use if `line-number-mode' is
> > > > enabled, but I can't find it again now. (Someone should build an email
> > > > client that allows easy searching.)
> > > 
> > > Do you perhaps mean this: id:"87k47nlpzb.fsf@nausicaa.localdomain"
> > 
> > [Now also to the list. Got bitten by my own patch binding 'r' to
> > reply-to-sender. Perhaps it's really too drastic to change that now...]
> 
> Yes, that one.
> 
> Ivy: Are you likely to add a test and re-submit the patch?

Sorry, I've gone back to using mutt. Notmuch is a pretty
cool project, but ultimately mail archiving is just not a
tool I need right now.

Good luck, though!

Ivy

Re: [PATCH] emacs: Improve `notmuch-hello' display on ttys.

[Xavier Maillard]

On Tue, 10 Jan 2012 10:15:28 +0000, David Edmondson <dme@dme.org> wrote:
> Inserting spaces to pad out columns is good, except when the padding
> makes the line wider than the window. This looks particularly bad on a
> tty where there is no fringe.
> 
> Hence, avoid padding the last column on each row.

Works for me

/Xavier

Re: [PATCH] emacs: Improve `notmuch-hello' display on ttys.

[David Bremner]

On Tue, 10 Jan 2012 10:15:28 +0000, David Edmondson <dme@dme.org> wrote:
> Inserting spaces to pad out columns is good, except when the padding
> makes the line wider than the window. This looks particularly bad on a
> tty where there is no fringe.

Pushed, 

d