Skip to content

Commit

Permalink
patch 8.1.1748: :args output is not aligned
Browse files Browse the repository at this point in the history 
Problem:    :args output is not aligned.
Solution:   Output a line break after the last item in a row.
  • Loading branch information
@brammool
brammool committed Jul 25, 2019
1 parent fbfb757 commit 74da393
Showing 1 changed file with 9 additions and 0 deletions.
9 changes: 9 additions & 0 deletions src/version.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -777,6 +777,8 @@ static char *(features[]) =

static int included_patches[] =
{ /* Add new patch number below this line */
/**/
1748,
/**/
1747,
/**/
Expand Down Expand Up @@ -4440,6 +4442,13 @@ list_in_columns(char_u **items, int size, int current)
msg_putchar(' ');
}
}
else
{
// this row is out of items, thus at the end of the row
if (msg_col > 0 && cur_row < nrow)
msg_putchar('\n');
++cur_row;
}
}
}

Expand Down

5 comments on commit 74da393

@blueyed
Copy link

@blueyed blueyed commented on 74da393 Jul 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it still appears to not be good:
I am seeing this with vim $(seq 1 80) (and COLUMNS=79 - but with regard to later comments it might have been 78 instead actually):

[1] 6   11  16  21  26  31  36  41  46  51  56  61  66  71  76
2   7   12  17  22  27  32  37  42  47  52  57  62  67  72  77
3   8   13  18  23  28  33  38  43  48  53  58  63  68  73  78  4   9   14  1
9   24  29  34  39  44  49  54  59  64  69  74  79  5   10  15  20  25  30  3
5   40  45  50  55  60  65  70  75  80

4 is not in order, 9 appears twice..

@blueyed
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also an trailing newline with args being longer than the available width (e.g. with :exe 'args '.repeat('X', &columns+1)).

This fixes it:

diff --git i/src/version.c w/src/version.c
index 8518667e9..3cfe53119 100644
--- i/src/version.c
+++ w/src/version.c
@@ -4414,8 +4414,9 @@ list_in_columns(char_u **items, int size, int current)
        for (i = 0; i < item_count; ++i)
        {
            version_msg_wrap(items[i], i == current);
-           if (msg_col > 0)
+           if (msg_col > 0 && i < item_count - 1) {
                msg_putchar('\n');
+           }
        }
        return;
     }

@blueyed
Copy link

@blueyed blueyed commented on 74da393 Jul 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried the first issue again (COLUMNS=79):

[1] 5   9   13  17  21  25  29  33  37  41  45  49  53  57  61  65  69  73  77
2   6   10  14  18  22  26  30  34  38  42  46  50  54  58  62  66  70  74  78
3   7   11  15  19  23  27  31  35  39  43  47  51  55  59  63  67  71  75  79
4   8   12  16  20  24  28  32  36  40  44  48  52  56  60  64  68  72  76  80

With 1 .. 81 however:

[1] 6   11  16  21  26  31  36  41  46  51  56  61  66  71  76  81
2   7   12  17  22  27  32  37  42  47  52  57  62  67  72  77
3   8   13  18  23  28  33  38  43  48  53  58  63  68  73  78  4   9   14  19
24  29  34  39  44  49  54  59  64  69  74  79  5   10  15  20  25  30  35  40
45  50  55  60  65  70  75  80

ncol = 20
nrow = 5

But only rows 3 and 4 have 20 items, and looking at in gdb showed that cur_row was getting too big.

This fixes that:

diff --git i/src/version.c w/src/version.c
index 8518667e9..00bed7c07 100644
--- i/src/version.c
+++ w/src/version.c
@@ -4459,9 +4460,11 @@ list_in_columns(char_u **items, int size, int current)
        else
        {
            // this row is out of items, thus at the end of the row
-           if (msg_col > 0 && cur_row < nrow)
-               msg_putchar('\n');
-           ++cur_row;
+           if (msg_col > 0) {
+               if (cur_row < nrow)
+                   msg_putchar('\n');
+               ++cur_row;
+           }
        }
     }
 }

But it is not clear to me what the initial patch here was fixing, and if it is still fixed.

Having tests for those would be good therefore.

Result:

[1] 6   11  16  21  26  31  36  41  46  51  56  61  66  71  76  81
2   7   12  17  22  27  32  37  42  47  52  57  62  67  72  77
3   8   13  18  23  28  33  38  43  48  53  58  63  68  73  78
4   9   14  19  24  29  34  39  44  49  54  59  64  69  74  79
5   10  15  20  25  30  35  40  45  50  55  60  65  70  75  80

@tom-m
Copy link

@tom-m tom-m commented on 74da393 Jul 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial patch was suggested by me. I set out today to write a test for it. And I discovered that there was still a bug in it. I was going to get fresh sources from the repo - only to find out that you already discovered and fixed the outstanding problem.

The thing that was fixed by the initial patch is still fixed after your fix. It was a situation where the last 2 of the 3 fields in the last column of the table were empty (idx >= item_count). The point was that it was more than 1. (You can see it on vim_dev). The test you brought with your follow-up fix deals with 19 empty fields. Specifically all of the last column and some more. That's even more than my 2 empty fields. That covers my situation too. So there is no need to add a test for it anymore. If you still want to check it, it was going to be based (as noted on vim_dev) on this:

:let width = &columns / 3 - 4
:let farg = repeat('f', width)
:let garg = repeat('g', width)
:exe 'args a b c d e' farg garg
:args

Which still works:

[a]                    d                      gggggggggggggggggggggg
b                      e
c                      ffffffffffffffffffffff

@blueyed
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tom-m
Thanks for the followup.
I've wondered myself if the test could be more concise (given that it was just from what I've tested, not trimmed), but I am glad that it covers all cases now though at least.

Please sign in to comment.