[neomutt-devel] Header::collapsed and single message

[Floyd Anderson]

On Tue, 13 Mar 2018 16:14:42 +0000
Richard Russon <rich at flatcap.org> wrote:
>Hi Floyd,
>
>Sorry for the slow reply

Really no reason for a sorry, I’m happy if this list is alive for more 
than release announcements. ;-)

>
>> Currently I’m trying to figure out why OP_JUMP doesn’t work as expected
>
>OK, I've had a quick look.
>
> […]
>
>I've put a proof-of-concent in branch [devel/jump].

Thank you to come up with a complete solution related to the OP_JUMP 
behaviour, even if I’m only asked for a tip how to (logically) deal with 
Header::collapsed to determine whether a message is a or has children. 
That seems to be impossible currently, right?
>
>There's no error checking and it might the wrong solution in the wrong
>place.  But, it works.

Unfortunately it doesn’t! I’ve tried it and even the target message jump 
up is correct now, it:

  - segfaults when jumping to the very last message in index menu
  - unconditionally uncollapse every thread even it’s not necessary,
    e.g. when jumping to a top thread message within index menu
  - anticipate a misleading error message:
        "Root message is not visible in this limited view."
    from mutt_parent_message() method, instead of raising error:
        "That message is not visible."
    in OP_JUMP case branch itself
  - close the pager menu when the user provides no or an invalid number,
    instead of just bail out with an appropriate error message
  - confuse or vanish the menu/help and/or index line in limited views
    when nonsense is given as message number – so invoking <refresh> is
    necessary afterwards


-- 
Regards,
floyd


Annotations for the patch below:

I’ve tested it a lot and found no wrong behaviour. I think it is more 
readable as the upstream Mutt code and also more compact (thanks to some 
NeoMutt extensions).

But what keeps bothering me is that I cannot just do:

    if (hdr->collapsed)

instead of:

    if (hdr->virtual != mutt_parent_message(Context, hdr, 1))

which was the main intention to ask for on the neomutt-dev list. I think 
it’s hard to figure out that the current condition just test whether the 
jump target message is a or has thread children.

Note, there is still an issue when trying to jump to a message by using
no or an invalid message number in pager menu. Normally an appropriate
error message is printed and current message is redisplayed afterwards.

But redisplaying a message that also raises an error message by itself,
e.g. "S/MIME signature could NOT be verified." or similar, will vanish
the origin error message and that might confuse the user.
---
 curs_main.c | 61 +++++++++++++++++--------------------------------------------
 1 file changed, 17 insertions(+), 44 deletions(-)

diff --git a/curs_main.c b/curs_main.c
index 87c4ce33..16439e73 100644
--- a/curs_main.c
+++ b/curs_main.c
@@ -1413,58 +1413,31 @@ int mutt_index_menu(void)
           mutt_unget_event(LastKey, 0);
         buf[0] = 0;
         if (mutt_get_field(_("Jump to message: "), buf, sizeof(buf), 0) != 0 || !buf[0])
+          mutt_error(_("Nothing to do."));
+        else if (mutt_str_atoi(buf, &i) < 0)
+          mutt_error(_("Argument must be a message number."));
+        else if (i < 1 || Context->msgcount < i)
+          mutt_error(_("Invalid message number."));
+        else if (!message_is_visible(Context, i - 1))
+          mutt_error(_("That message is not visible."));
+        else
         {
-          if (menu->menu == MENU_PAGER)
+          struct Header *hdr = Context->hdrs[i - 1];
+          if (hdr->virtual != mutt_parent_message(Context, hdr, 1))
           {
-            op = OP_DISPLAY_MESSAGE;
-            continue;
+            mutt_uncollapse_thread(Context, hdr);
+            mutt_set_virtual(Context);
           }
-          break;
-        }
-
-        if (mutt_str_atoi(buf, &i) < 0)
-        {
-          mutt_error(_("Argument must be a message number."));
-          break;
+          menu->current = hdr->virtual;
         }

-        if (i > 0 && i <= Context->msgcount)
+        if (menu->menu == MENU_PAGER)
         {
-          int msg = mutt_parent_message(Context, Context->hdrs[i], 1);
-          msg = Context->v2r[msg];
-          mutt_uncollapse_thread(Context, Context->hdrs[msg]);
-          mutt_set_virtual(Context);
-
-          for (j = i - 1; j < Context->msgcount; j++)
-          {
-            if (Context->hdrs[j]->virtual != -1)
-              break;
-          }
-          if (j >= Context->msgcount)
-          {
-            for (j = i - 2; j >= 0; j--)
-            {
-              if (Context->hdrs[j]->virtual != -1)
-                break;
-            }
-          }
-
-          if (j >= 0)
-          {
-            menu->current = Context->hdrs[j]->virtual;
-            if (menu->menu == MENU_PAGER)
-            {
-              op = OP_DISPLAY_MESSAGE;
-              continue;
-            }
-            else
-              menu->redraw = REDRAW_FULL;
-          }
-          else
-            mutt_error(_("That message is not visible."));
+          op = OP_DISPLAY_MESSAGE;
+          continue;
         }
         else
-          mutt_error(_("Invalid message number."));
+          menu->redraw = REDRAW_FULL;

         break;

-- 
2.16.2