[PATCH] Improve moving between messages in a thread

[PATCH] Improve moving between messages in a thread

[Ian Main]

This patch adds a few changes to moving between threads:

- It supports 'scrolloff' so that if you have this set it will move the
  buffer and cursor so the next/prev email starts at the top of the
  screen.
- It adds the ability to use shift-tab to go to the previous msg in
  the thread.

    Ian
---
 vim/notmuch.vim | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/vim/notmuch.vim b/vim/notmuch.vim
index 331e930..95e5c4b 100644
--- a/vim/notmuch.vim
+++ b/vim/notmuch.vim
@@ -39,6 +39,7 @@ let g:notmuch_show_maps = {
 	\ 'p':		'show_save_patches()',
 	\ 'r':		'show_reply()',
 	\ '?':		'show_info()',
+	\ '<S-Tab>':	'show_prev_msg()',
 	\ '<Tab>':	'show_next_msg()',
 	\ 'c':		'compose()',
 	\ }
@@ -113,6 +114,23 @@ EOF
 	call s:kill_this_buffer()
 endfunction
 
+function! s:show_prev_msg()
+ruby << EOF
+	r, c = $curwin.cursor
+	n = $curbuf.line_number
+	i = $messages.index { |m| n >= m.start && n <= m.end }
+	m = $messages[i - 1] if i > 0
+	vim_puts ("messages index is #{i} and m is #{m}")
+	if m
+		r = m.body_start + 1
+		scrolloff = VIM::evaluate("&scrolloff")
+		VIM::command("normal #{m.start + scrolloff}zt")
+		$curwin.cursor = r + scrolloff, c
+		vim_puts("moving to #{m.start + scrolloff}")
+	end
+EOF
+endfunction
+
 function! s:show_next_msg()
 ruby << EOF
 	r, c = $curwin.cursor
@@ -121,8 +139,10 @@ ruby << EOF
 	m = $messages[i + 1]
 	if m
 		r = m.body_start + 1
-		VIM::command("normal #{m.start}zt")
-		$curwin.cursor = r, c
+		scrolloff = VIM::evaluate("&scrolloff")
+		VIM::command("normal #{m.start + scrolloff}zt")
+		$curwin.cursor = r + scrolloff, c
+		vim_puts("moving to #{m.start + scrolloff}")
 	end
 EOF
 endfunction
-- 
1.9.3

RE: [PATCH] Improve moving between messages in a thread

[Ian Main]

Ian Main wrote:
> This patch adds a few changes to moving between threads:
> 
> - It supports 'scrolloff' so that if you have this set it will move the
>   buffer and cursor so the next/prev email starts at the top of the
>   screen.
> - It adds the ability to use shift-tab to go to the previous msg in
>   the thread.
> 
>     Ian
> ---
>  vim/notmuch.vim | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/vim/notmuch.vim b/vim/notmuch.vim
> index 331e930..95e5c4b 100644
> --- a/vim/notmuch.vim
> +++ b/vim/notmuch.vim
> @@ -39,6 +39,7 @@ let g:notmuch_show_maps = {
>  	\ 'p':		'show_save_patches()',
>  	\ 'r':		'show_reply()',
>  	\ '?':		'show_info()',
> +	\ '<S-Tab>':	'show_prev_msg()',
>  	\ '<Tab>':	'show_next_msg()',
>  	\ 'c':		'compose()',
>  	\ }
> @@ -113,6 +114,23 @@ EOF
>  	call s:kill_this_buffer()
>  endfunction
>  
> +function! s:show_prev_msg()
> +ruby << EOF
> +	r, c = $curwin.cursor
> +	n = $curbuf.line_number
> +	i = $messages.index { |m| n >= m.start && n <= m.end }
> +	m = $messages[i - 1] if i > 0
> +	vim_puts ("messages index is #{i} and m is #{m}")
> +	if m
> +		r = m.body_start + 1
> +		scrolloff = VIM::evaluate("&scrolloff")
> +		VIM::command("normal #{m.start + scrolloff}zt")
> +		$curwin.cursor = r + scrolloff, c
> +		vim_puts("moving to #{m.start + scrolloff}")

Woops, forgot to remove the vim_puts debugging.

    Ian

[PATCH] VIM: Improve moving between messages in a thread

[Ian Main]

Add a few changes to moving between threads:

- It supports 'scrolloff' so that if you have this set it will move the
  buffer and cursor so the next/prev email starts at the top of the
  screen.
- It adds the ability to use shift-tab to go to the previous msg in
  the thread.

    Ian
---

This update fixes the commit message and removes vim_puts debugging.

 vim/notmuch.vim | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/vim/notmuch.vim b/vim/notmuch.vim
index 331e930..2124a8e 100644
--- a/vim/notmuch.vim
+++ b/vim/notmuch.vim
@@ -39,6 +39,7 @@ let g:notmuch_show_maps = {
 	\ 'p':		'show_save_patches()',
 	\ 'r':		'show_reply()',
 	\ '?':		'show_info()',
+	\ '<S-Tab>':	'show_prev_msg()',
 	\ '<Tab>':	'show_next_msg()',
 	\ 'c':		'compose()',
 	\ }
@@ -113,6 +114,22 @@ EOF
 	call s:kill_this_buffer()
 endfunction
 
+function! s:show_prev_msg()
+ruby << EOF
+	r, c = $curwin.cursor
+	n = $curbuf.line_number
+	i = $messages.index { |m| n >= m.start && n <= m.end }
+	m = $messages[i - 1] if i > 0
+	vim_puts ("messages index is #{i} and m is #{m}")
+	if m
+		r = m.body_start + 1
+		scrolloff = VIM::evaluate("&scrolloff")
+		VIM::command("normal #{m.start + scrolloff}zt")
+		$curwin.cursor = r + scrolloff, c
+	end
+EOF
+endfunction
+
 function! s:show_next_msg()
 ruby << EOF
 	r, c = $curwin.cursor
@@ -121,8 +138,9 @@ ruby << EOF
 	m = $messages[i + 1]
 	if m
 		r = m.body_start + 1
-		VIM::command("normal #{m.start}zt")
-		$curwin.cursor = r, c
+		scrolloff = VIM::evaluate("&scrolloff")
+		VIM::command("normal #{m.start + scrolloff}zt")
+		$curwin.cursor = r + scrolloff, c
 	end
 EOF
 endfunction
-- 
1.9.3

Re: [PATCH] VIM: Improve moving between messages in a thread

[Franz Fellner]

Patch works fine for me. It also would be nice to have a "move to next
unread message" function.
Most beautiful would be a treeview of the thread structure. Currently
threads are rendered as plain list, so you can't immediately see and
jump to the quoted mail. Bower IMHO uses the best approach here. I tried
emacs treeview but did not really like it (partly because I failed badly
to implement a solution that shows treeview by default). Indent the
whole large Message view IMHO also is not a good (at least to me)
solution.

On Mon,  6 Oct 2014 10:55:16 -0700, Ian Main <imain@stemwinder.org> wrote:
> @@ -113,6 +114,22 @@ EOF
>  	call s:kill_this_buffer()
>  endfunction
>  
> +function! s:show_prev_msg()
> +ruby << EOF
> +	r, c = $curwin.cursor
> +	n = $curbuf.line_number
> +	i = $messages.index { |m| n >= m.start && n <= m.end }
> +	m = $messages[i - 1] if i > 0

This one
> +	vim_puts ("messages index is #{i} and m is #{m}")
was missed ;)

> +	if m
> +		r = m.body_start + 1
> +		scrolloff = VIM::evaluate("&scrolloff")
> +		VIM::command("normal #{m.start + scrolloff}zt")
> +		$curwin.cursor = r + scrolloff, c
> +	end
> +EOF
> +endfunction
> +
>  function! s:show_next_msg()
>  ruby << EOF
>  	r, c = $curwin.cursor
> @@ -121,8 +138,9 @@ ruby << EOF
>  	m = $messages[i + 1]
>  	if m
>  		r = m.body_start + 1
> -		VIM::command("normal #{m.start}zt")
> -		$curwin.cursor = r, c
> +		scrolloff = VIM::evaluate("&scrolloff")
> +		VIM::command("normal #{m.start + scrolloff}zt")
> +		$curwin.cursor = r + scrolloff, c
>  	end
>  EOF
>  endfunction
> -- 
> 1.9.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] VIM: Improve moving between messages in a thread

[Ian Main]

Franz Fellner wrote:
> Patch works fine for me. It also would be nice to have a "move to next
> unread message" function.
> Most beautiful would be a treeview of the thread structure. Currently
> threads are rendered as plain list, so you can't immediately see and
> jump to the quoted mail. Bower IMHO uses the best approach here. I tried
> emacs treeview but did not really like it (partly because I failed badly
> to implement a solution that shows treeview by default). Indent the
> whole large Message view IMHO also is not a good (at least to me)
> solution.

Yeah I agree.  This is a start anyway.  As I use the vim client more my
plan is to just keep fixing things that get in my way.

Thanks for catching the vim_puts. :)

Thanks for you're reviews!!

    Ian

> On Mon,  6 Oct 2014 10:55:16 -0700, Ian Main <imain@stemwinder.org> wrote:
> > @@ -113,6 +114,22 @@ EOF
> >  	call s:kill_this_buffer()
> >  endfunction
> >  
> > +function! s:show_prev_msg()
> > +ruby << EOF
> > +	r, c = $curwin.cursor
> > +	n = $curbuf.line_number
> > +	i = $messages.index { |m| n >= m.start && n <= m.end }
> > +	m = $messages[i - 1] if i > 0
> 
> This one
> > +	vim_puts ("messages index is #{i} and m is #{m}")
> was missed ;)
> 
> > +	if m
> > +		r = m.body_start + 1
> > +		scrolloff = VIM::evaluate("&scrolloff")
> > +		VIM::command("normal #{m.start + scrolloff}zt")
> > +		$curwin.cursor = r + scrolloff, c
> > +	end
> > +EOF
> > +endfunction
> > +
> >  function! s:show_next_msg()
> >  ruby << EOF
> >  	r, c = $curwin.cursor
> > @@ -121,8 +138,9 @@ ruby << EOF
> >  	m = $messages[i + 1]
> >  	if m
> >  		r = m.body_start + 1
> > -		VIM::command("normal #{m.start}zt")
> > -		$curwin.cursor = r, c
> > +		scrolloff = VIM::evaluate("&scrolloff")
> > +		VIM::command("normal #{m.start + scrolloff}zt")
> > +		$curwin.cursor = r + scrolloff, c
> >  	end
> >  EOF
> >  endfunction
> > -- 
> > 1.9.3
> > 
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] VIM: Improve moving between messages in a thread

[Franz Fellner]

Ian Main wrote:
> Franz Fellner wrote:
> > Patch works fine for me. It also would be nice to have a "move to next
> > unread message" function.
> > Most beautiful would be a treeview of the thread structure. Currently
> > threads are rendered as plain list, so you can't immediately see and
> > jump to the quoted mail. Bower IMHO uses the best approach here. I tried
> > emacs treeview but did not really like it (partly because I failed badly
> > to implement a solution that shows treeview by default). Indent the
> > whole large Message view IMHO also is not a good (at least to me)
> > solution.
> 
> Yeah I agree.  This is a start anyway.  As I use the vim client more my
> plan is to just keep fixing things that get in my way.

Great :)

Just a LGTM to get this pushed.

Franz

Re: [PATCH] VIM: Improve moving between messages in a thread

[Tomi Ollila]

On Sat, Oct 18 2014, Franz Fellner <alpine.art.de@gmail.com> wrote:

> Ian Main wrote:
>> Franz Fellner wrote:
>> > Patch works fine for me. It also would be nice to have a "move to next
>> > unread message" function.
>> > Most beautiful would be a treeview of the thread structure. Currently
>> > threads are rendered as plain list, so you can't immediately see and
>> > jump to the quoted mail. Bower IMHO uses the best approach here. I tried
>> > emacs treeview but did not really like it (partly because I failed badly
>> > to implement a solution that shows treeview by default). Indent the
>> > whole large Message view IMHO also is not a good (at least to me)
>> > solution.
>> 
>> Yeah I agree.  This is a start anyway.  As I use the vim client more my
>> plan is to just keep fixing things that get in my way.
>
> Great :)
>
> Just a LGTM to get this pushed.

What was this 'vim_puts missed' comment in this thread -- I did not see any
followup patch that referred to it -- any potential reviewer may wonder the
same...

>
> Franz

Tomi

[PATCH v3] VIM: Improve moving between messages in a thread

[Ian Main]

Add a few changes to moving between threads:

- It supports 'scrolloff' so that if you have this set it will move the
  buffer and cursor so the next/prev email starts at the top of the
  screen.
- It adds the ability to use shift-tab to go to the previous msg in
  the thread.

    Ian
---

Remove spurious vim_puts.

 vim/notmuch.vim | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/vim/notmuch.vim b/vim/notmuch.vim
index 331e930..bd007b1 100644
--- a/vim/notmuch.vim
+++ b/vim/notmuch.vim
@@ -39,6 +39,7 @@ let g:notmuch_show_maps = {
 	\ 'p':		'show_save_patches()',
 	\ 'r':		'show_reply()',
 	\ '?':		'show_info()',
+	\ '<S-Tab>':	'show_prev_msg()',
 	\ '<Tab>':	'show_next_msg()',
 	\ 'c':		'compose()',
 	\ }
@@ -113,6 +114,21 @@ EOF
 	call s:kill_this_buffer()
 endfunction
 
+function! s:show_prev_msg()
+ruby << EOF
+	r, c = $curwin.cursor
+	n = $curbuf.line_number
+	i = $messages.index { |m| n >= m.start && n <= m.end }
+	m = $messages[i - 1] if i > 0
+	if m
+		r = m.body_start + 1
+		scrolloff = VIM::evaluate("&scrolloff")
+		VIM::command("normal #{m.start + scrolloff}zt")
+		$curwin.cursor = r + scrolloff, c
+	end
+EOF
+endfunction
+
 function! s:show_next_msg()
 ruby << EOF
 	r, c = $curwin.cursor
@@ -121,8 +137,9 @@ ruby << EOF
 	m = $messages[i + 1]
 	if m
 		r = m.body_start + 1
-		VIM::command("normal #{m.start}zt")
-		$curwin.cursor = r, c
+		scrolloff = VIM::evaluate("&scrolloff")
+		VIM::command("normal #{m.start + scrolloff}zt")
+		$curwin.cursor = r + scrolloff, c
 	end
 EOF
 endfunction
-- 
1.9.3

RE: [PATCH v3] VIM: Improve moving between messages in a thread

[Bartosz Telenczuk]

Hi Ian,

The patch looks good. I tested it on my system and it works fine.  I just have one suggestion regarding coding style.

> +function! s:show_prev_msg()
>  function! s:show_next_msg()

Since these functions are almost the same, you could avoid code repetition by replacing them by a function which takes (positive or negative) increment as an argument.

Cheers,

Bartosz

RE: [PATCH v3] VIM: Improve moving between messages in a thread

[Franz Fellner]

Bartosz Telenczuk wrote:
> Hi Ian,
> 
> The patch looks good. I tested it on my system and it works fine.  I just have one suggestion regarding coding style.
> 
> > +function! s:show_prev_msg()
> >  function! s:show_next_msg()
> 
> Since these functions are almost the same, you could avoid code repetition by replacing them by a function which takes (positive or negative) increment as an argument.

I think the API should be clear and easy to understand, because people will probably bind them to custom shortcuts. You should not have to think about what that weird extra parameter (-1, false, ...) means and go read the docs.
But I agree that the code duplication is not optimal. The code in Ians repo already has split out the ruby code. There is still code duplication. But that can easily be split out into a function on its own. 

Franz

RE: [PATCH v3] VIM: Improve moving between messages in a thread

[Bartosz Telenczuk]

> I think the API should be clear and easy to understand, because people will probably bind them to custom shortcuts. You should not have to think about what that weird extra parameter (-1, false, ...) means and go read the docs.

I agree. One might replace the parameter with a string like "prev" and "next".

> But I agree that the code duplication is not optimal. The code in Ians repo already has split out the ruby code. There is still code duplication. But that can easily be split out into a function on its own. 

That's the way to go.

Bartosz