[RDY] statusline: unlimited alignment sections #4489

Closed
wants to merge 1 commit into
from

Projects

None yet

10 participants

@tjdevries
Contributor
tjdevries commented Mar 23, 2016 edited

Current Status:

I think this feature is complete and I have added a lot of tests after discussing with other contributors here. The current problem is just getting this seemingly unrelated test to stop failing.

Pictures

Basic Example

basic_in_the_middle

More sections

good_multiple_sections

This is in regards to the enhancement request made on #4454.

Summary

  • Implement main features
    • Modify STL Marker STL_MIDDLEMARK to STL_SEPARATE
      • Remove mentions of STL_MIDDLEMARK
        • I can't seem to figure out how to remove mentions that are generated in auto.
      • Update to mentions of STL_SEPARATE
      • Add the STL Marker to STL_ALL
    • Correctly identify the new marker in build_stl_str_hl
    • Correctly format the results with the modified marker in build_stl_str_hl
    • Truncation
  • Testing
    • Add basic tests for the new formatting option
    • Add tests for all features with x's above.
  • Documentation
    • Update options.txt
    • Add example pictures of basic features
    • Update any other related documentation (need some suggestions here)

Other Items (Not sure if they should be in this Pull Request?)

  • Add more tests for the original features of build_stl_str_hl.
    • Separate the test buffers for each unit test
@marvim marvim added the WIP label Mar 23, 2016
@tjdevries tjdevries changed the title from [WIP] Center Items in the statusline / tagline to [RFC] Center Items in the statusline / tagline Mar 30, 2016
@marvim marvim added RFC and removed WIP labels Mar 30, 2016
@tjdevries
Contributor

@justinmk, is there anyone would be particularly well suited to look at this PR?

@jamessan jamessan and 1 other commented on an outdated diff Mar 31, 2016
src/nvim/buffer.c
+ if (item[item_idx].type == CenterEnd && center_end == -1) {
+ center_end = item_idx;
+ }
+ if (item[item_idx].type == Middle && middle_mark == -1) {
+ middle_mark = item_idx;
+ }
+ }
+
+ // Only counts as a valid center if both pieces are found.
+ if (center_begin >= 0 && center_end >= 0) {
+ center = true;
+ }
+
+ // If we have a center area, then we deal with it now
+ if (center) {
+ // This is where we handle if the middle mark is presetn as well
@jamessan
jamessan Mar 31, 2016 Member

Typo: presetn -> present

@tjdevries
tjdevries Mar 31, 2016 Contributor

When I fix this, what is the best practice for updating. Do I just push a new commit (that I eventually squash down into more sensible commits) to the PR?

@jamessan jamessan and 1 other commented on an outdated diff Mar 31, 2016
src/nvim/buffer.c
+ // Only counts as a valid center if both pieces are found.
+ if (center_begin >= 0 && center_end >= 0) {
+ center = true;
+ }
+
+ // If we have a center area, then we deal with it now
+ if (center) {
+ // This is where we handle if the middle mark is presetn as well
+ if (middle_mark > 0) {
+ // Handle a combination of middle mark and centering
+ //
+ // a b c d
+ // text\ %[this\ text\ here%=\ and\ some%]finishing
+ // text his text here and some finishing
+ // quad1 quad2|quad3 quad4
+ // --fill1-- --fill2--
@jamessan
jamessan Mar 31, 2016 Member

This is a handy diagram. It looks like you intended to have some of the variables used below reflected in the diagram, but the names don't match up. For example, is fill1 supposed to be the same as fill_left?

@tjdevries
tjdevries Mar 31, 2016 Contributor

Yeah, I can rename some of those variables in the diagram to be what I ended up deciding on the names. Thanks.

@justinmk
Member

@tjdevries The direction you're going with presenting a design and writing tests is optimal. It makes it super easy for me or others to come in near the end and look for gaps, fuzz-test the tests, think about missing tests, and comment on possible "best practices".

I feel really good about adding features that have good test coverage, even if I don't care about the feature, as long as they don't make the code much more complicated.

@justinmk justinmk and 2 others commented on an outdated diff Mar 31, 2016
src/nvim/buffer.c
+ item[item_idx].start += center_begin_fill;
+ }
+
+ char_u *end = item[center_end].start + center_end_fill;
+ STRMOVE(end, item[center_end].start);
+
+ // Fill the <text>]<text to be <text> <text>
+ for (char_u *s = item[center_end].start; s < end; s++) {
+ *s = fillchar;
+ }
+
+ width = maxwidth;
+ }
+ // Otherwise, we just do the exact same thing with the middle that
+ // we used to do.
+ } else {
@justinmk
justinmk Mar 31, 2016 Member

You added a huge block of code to build_stl_str_hl, consider moving it to a private (static) function(s).

@tjdevries
tjdevries Mar 31, 2016 Contributor

So it would be acceptable to break down some of the actions that I've done into smaller, more compact function calls? I wasn't sure if that was following the same practice here, as all of the other handling of the STL_{markers} is done within that function (as far as I can tell).

I prefer it the way you are suggesting, as it would allow me to unit test some of the ideas better and make me feel a lot better about the overall code.

@jamessan
jamessan Mar 31, 2016 Member

I don't think this code has been touched much other than reformatting, so it still follows much of Vim's style.

New code should definitely be less monolithic.

@tjdevries
tjdevries Mar 31, 2016 Contributor

Okay, that's nice to hear. I will go back and split a lot of this functionality out into more manageable functions. I can just ping both of you ( @jamessan & @justinmk ) once that's completed? It may take me a bit because I'd also like to add some unit tests for those functions if that's alright.

@tjdevries
tjdevries Mar 31, 2016 Contributor

@justinmk, is it possible to unit test the private (static) functions with the Lua functional/unit tests? what would be the recommended way to do that?

@tjdevries
Contributor

@ZyX-I, I just found that you added, at least in the tabline, a marker already using %[. Will what I've done here conflict with that at all? I can do some more experimenting, or change the marker I'm using. I didn't see that when I started, but just found it now while digging.

I found it in this commit:ef66249

It looks like that got changed now though. It is now %@? Is that true. Sorry, I think I got confused cause I was looking at an older commit for that part of it.

@joshaw
joshaw commented Apr 1, 2016

I apologise if I'm speaking out of place, or should have commented earlier, feel free to disregard.

Would it make more sense, given your remark above about possible conflict with another statusline related flag (and going forward when more may be added), to re-use the existing %= marker? It seems that using %= as a "expanding separator" would allow backwards compatibility and forwards innovation. So instead of the traditional:

left hand side %= right hand side

with your exension:

left side %[ middle %] right side

we could use:

left side %= middle %= right side

which would allow, if desired:

left %= left 1 %= middle %= middle 2 %= right %= right 3

I admit to not having understood, or even more than briefly perused, the code, so this may not be possible with the framework in place, but worth considering?

@tjdevries
Contributor

@joshaw, Thanks for the suggestion. I don't think the conflict actually exists anymore after looking more into it, now I am just checking to make sure that the conflict was fully phased out.

While that could be nice, there are two problems with your suggestion I think.

The first is that it would be extremely difficult to code (I'm not entirely sure I'm the person for that job either, as it was difficult enough to get it centered as is).

The second is that, at least I can't think of a way to do it, to have something like this happen:

Statusline:
left side %[ middle left side %=%] right side

Result (I didn't count the spaces, but you get the idea I think):
left side_____middle left_______________right side

I'm not really sure how you could make that work with

left side %= middle left side %=%= right side.

Not to mention truncation.

So I think it's a really good idea. There's potential that you could use %= to work for up to the quadrants approach that I have above, and if some of the other Neovim pros think that would work better, I think I could make that work. TBH, I prefer the explicit starting and ending of centering as it makes sense when I look at a statusline to see what it does:

This set statusline=left%[centering%]right just makes a lot of sense to me.

Thanks again for the feedback 😄

@tjdevries
Contributor

I have a couple projects picking up at school, so I will have to put this on hold for a bit. I am also going to try and see if I can get this patch discussed at vim-dev, so that it can potentially be released to the community at large. I'll still monitor the thread though if anyone has suggestions in the mean time.

@ZyX-I
Contributor
ZyX-I commented Apr 4, 2016

@tjdevries It is not marker that changed: %[… was used for “run-command-on-click”. I then found solution which should be more convenient and released %[ because new solution does not need pairing brackets and there may be some other idea that uses them (e.g. like your one). Though I personally would not think of a new marker for your PR at all, just “left%=center%=right”. For truncation there is %<: “left%=%<center, truncated at start%=right”. And to make this more generic: “left%=center1%=center2%=right” should mean that any free space left is distributed more or less equally between %=, which is the common property for left%=right, left%=center%=right and left%=center1%=center2%=right, with proper code you should have no problems in supporting any number of sections.

@ZyX-I
Contributor
ZyX-I commented Apr 4, 2016

@tjdevries I also think that last sentence about %= has much greater chances to be accepted by Bram because this is more “conservative”: no new flags, no new flag meanings, just existing flags functionality slightly (from the user perspective) changed: %< does not need to have documentation changed at all, %= changes from “Separation point between left and right aligned items.” to “Separation point between statusline sections. Free space is distributed equally between %= items: e.g. "left_aligned%=right_aligned" or "left_aligned%=centered%=right_aligned".”

By the way, your tests are incomplete. I have checked all of them and all look like {string with width N}%[…%]{string with width N}. You need also {N}%[…%]{M≠N} ({C} abbreviates {string with width C}), {N}%[…%], %[…%]{N} I would also highly suggest to replace all eq("abc ", string.sub(helpers.ffi.string(output_buffer, width), 1, 4))\neq(…, string.sub(…)) with eq("abc neovim def", ffi.string(output_buffer, width)), but in place of 80 use something else, current variant

  1. is not easy to write: you need to compute offsets
  2. not easy to review: statusline is a visual thing, correct offsets is nothing compared to correct view (especially as a reviewer I would be interested how truncation looks, not at what offset truncation symbol is placed)
  3. not easy to review: reviewer need to determine and recompute offsets
  4. does not test what also needs to be tested: what if spaces you do not test contain garbage because you missed initialization somewhere

And note: ffi.string, not helpers.ffi: most tests state that they are using specific symbol from helpers module at the top of the file.

@tjdevries
Contributor

@ZyX-I Thanks for all your comments! I see what you are saying about using %= for all of the marks. I will take a look at that and then having all of them represent equal spacing. Thank you also for your insight into having a better chance of Bram accepting this PR.

I do have a couple questions I'd like to get your opinion on.

  1. How would you attempt to get something like this to happen:
    • Statusline: left side %[ middle left side %=%]
    • Result: left side_____middle left_________________________
    • Options: left side%=middle left%=%=
    • But perhaps that is just a bad use case, and it isn't really necessary to have. In that case, we wouldn't have to worry about it.
  2. Another thing I was wondering about is whether each section between %= flags should have the same number of total spaces, or whether there should just be equal spaces between each section. As an example, if we have a 100 character width statusline, and my statusline format is set to quad1\ with\ text%=quad2%=quad3%=quad4, does quad2 start after 25 characters, or does it start after (100 - num_characters) / 3 + 15 (where 15 is the number of characters in quad1 with text)? I'm not sure which way would be more in line with the flags functionality.
    • The other main reason I ask this is because in the case where you have a statusline like left_text_that_is_very_long_and_still_long%=center%=right, that "centered" text won't really be in the center. It will be offset to the right because we are using equal spacing between the different sections, not equal sections of space.
  3. Thank you for your help with the tests. Would you suggest then just choosing a smaller size for the width and then comparing the entire expected string with the buffer? That does sound like a much better idea than what I currently had.
  4. I'm not sure what you mean by your last note. This was my first time using Lua, so I'm not familiar with all of the aspects of the language. Are you saying to not use helpers.ffi and stick with just ffi.string? I don't entirely understand the difference. Perhaps I will just read more about it or look at more tests.

Thanks again for all your help.

@tjdevries
Contributor

@ZyX-I, I have been thinking a lot about what you proposed early and mulling over the questions I asked above, and I do have a proposed solution:

  1. %= Now specifies the beginning of a new align_section (there may be a better name for this).
  2. Each section gets (approximately) the same width.
    • If the number of align_sections is odd, then the center align_section takes the extra spaces
    • Otherwise, they are split between the middle two sections.
  3. Text justification is desribed as follows:
    • If there is a center align_section, i.e. the third of five sections, that align_section will have centered text
    • All other align_sections that start on the left side of the center will have left justification.
    • All other align_sections that start on the right side of the center will have right justification.
  4. Truncation is on the right side when the align_section starts to the left of the center, and on the left side when the align_section.

Example

Suppose we had a buffer 34 characters wide, with indeces 0 - 33.

Our statusline is set as:

set statusline=first%=second_text%=middle%=fourth%=last_text

So we essentially have 5 align_sections. Each align section (besides the center, if odd) is maxwidth / align_sections. So in this case, each align_group has 34 / 5 = 6 spaces. The center align group will have 34 - ((5 - 1) * 6) = 10 spaces.

Index: ~ 0    5    10   15   20   25  30    ~
Space: ~ |____|____|____|____|____|____|__| ~
Group: ~ |-----|-----|---------|-----|----- ~
Start: ~ 0     1     2         3     4      ~  
Res  : ~ first secon>  middle  fourth<_text ~

So instead of focusing on having the exact same amount of spaces between each section, we make each section have the same amount of spaces. This should work well for any generic number of align_sections and provide most of the functionality I think people were looking for from the section.

In this method, there is no need to define a new marker.

What do you think?

I do still have some questions (that I mentioned above) about the tests and what would be the best way to write them, so if you could offer some advice in that regard, it would be really helpful.

@ZyX-I
Contributor
ZyX-I commented Apr 11, 2016

Main reason why I suggested “equal spacing” vs “equal width” was because this is how I would write such thing should I implement it in e.g. powerline. Main reasoning is that user (at least, me) will prefer displaying requested content over displaying that content centered. (In fact, I saw only a) tests and b) documentation and could not imagine that you have followed another approach: tests had flaws I described and I did not read code.)

I.e. if you have “equal width sections” and user appeared to not distribute content equally (especially between first and last in 3-section statusline) most lengthy section may appear truncated even if there is enough space. If you have “equal spacing” nothing will be truncated as long as there is space, but it may appear shifted. If content is distributed more or less equally between non-center sections then centered section will be centered enough for user to not mention it is not actually centered.

Also note that in

00000000001111111111222222222233333333334444444444555555555566666666667777777777
01234567890123456789012345678901234567890123456789012345678901234567890123456789
2 test/unit/buffer_spec.lua     1 options.txt+ 2                  unix utf-8 lua

2 test/unit/buffer_spec.lua            1 options.txt+ 2           unix utf-8 lua

visually centered is the second variant which is my “equal spacing”. Your variant looks like it have left-aligned central section while it is actually placed in the true statusline center.


About tests: tests are allowed to exceed 80-character limit. So if you feel that you need more lengthy string, just use them (though I would suggest to fit in 100- or 120-character limit). About helpers: I simply mean that you should not use helpers.xxx in test code. Near the top of the file there are many lines like

local foo = helpers.foo

they allow you to avoid using helpers.xxx anywhere, but at the top: below helpers.xxx will be just xxx. This is convention used in many tests (though you are not the first one to break it), not some language restriction. There is also no functional difference, it is code style convention. Also useful for refactoring: if e.g. ffi was moved out from helpers module one would need to change one line in each test, not every line where ffi is present.

@ZyX-I
Contributor
ZyX-I commented Apr 11, 2016

So as a summary to “equal width” sections there are two objections:

  1. Less content is shown in some cases.
  2. No visual advantages over “equal spacing”.

Though I do not have enough knowledge to prove the second. It just looks like my variant is better centered.

@tjdevries
Contributor

@ZyX-I, I see what you are saying. After examining my normal use cases for the statusline, I find that I usually have both sides reasonably similar in length. I think most importantly is that the most content is shown at all times, which I had not really considered before.

I will take a look at implementing the "equal width" %= marker.

And I understand what you're saying about the tests now. I will try and make them more visual as I work through the new setup.

Thanks for your help :)

@tjdevries
Contributor

Hmmm... It appears I messed up my fetch and rebase. Not sure exactly how to fix this. Sorry guys :/

I am looking right now.

@ZyX-I
Contributor
ZyX-I commented Apr 12, 2016

@tjdevries Use git reflog to find branch head commit before rebase.

@tjdevries
Contributor

Thanks, I think I fixed it. I was rebasing from the wrong branch was the main problem.

@brcolow brcolow commented on an outdated diff Apr 13, 2016
test/unit/buffer_spec.lua
+ buffer.setfname(globals.curbuf, to_cstr("test/unit/buffer_spec.lua"), NULL, 1)
+
+ local expected_width = 20
+ local width = build_stl_str_hl("abc%=neo_vim%=def", expected_width)
+
+ eq(width, expected_width)
+ eq("abc neo_vim def", get_str(output_buffer, expected_width))
+ end)
+
+ it('should correctly quadrant the text when using 3 %=\'s', function()
+ buffer.setfname(globals.curbuf, to_cstr("test/unit/buffer_spec.lua"), NULL, 1)
+
+ local expected_width = 40
+ local width = build_stl_str_hl("abc%=n%=eovim%=def", expected_width)
+
+ eq(width, width)
@brcolow
brcolow Apr 13, 2016 Contributor

Should this not be eq(width, expected_width) like the others?

@brcolow brcolow commented on an outdated diff Apr 13, 2016
test/unit/buffer_spec.lua
+ local expected_width = 40
+ local width = build_stl_str_hl("abc%=n%=eovim%=def", expected_width)
+
+ eq(width, width)
+ eq("abc n eovim def", get_str(output_buffer, expected_width))
+ end)
+
+ it('should truncate when standard text pattern is too long', function()
+ -- This is the standard way Vim truncates text when it is too long
+ buffer.setfname(globals.curbuf, to_cstr("test/unit/buffer_spec.lua"), NULL, 1)
+
+ local pat = "abcneovimdef"
+ local expected_width = 10
+ local width = build_stl_str_hl(pat, expected_width)
+
+ eq(width, width)
@brcolow
brcolow Apr 13, 2016 Contributor

This one too?

@ZyX-I ZyX-I commented on an outdated diff Apr 13, 2016
test/unit/buffer_spec.lua
- NULL,
- NULL)
- end
+ before_each(function()
+ buffer_width = 200
+ output_buffer = to_cstr(string.rep(" ", buffer_width))
+ -- Currently assumes fillchar = ' '
+
+ build_stl_str_hl = function(pat, maxwidth)
+ return buffer.build_stl_str_hl(globals.curwin,
+ output_buffer,
+ buffer_width,
+ to_cstr(pat),
+ false,
+ 32,
+ maxwidth,
@ZyX-I
ZyX-I Apr 13, 2016 Contributor

maxwidth or 80 and you do not need to specify maxwidth explicitly below.

@ZyX-I ZyX-I and 1 other commented on an outdated diff Apr 13, 2016
test/unit/buffer_spec.lua
+ -- It puts all of the text that is left aligned in the buffer,
+ -- and then truncates the text on the right.
+ eq("abcdef<jkl", get_str(output_buffer, expected_width))
+ end)
+
+ it('should truncate centered text when using %=%=', function()
+ -- Is this a desired behavior?
+ buffer.setfname(globals.curbuf, to_cstr("test/unit/buffer_spec.lua"), NULL, 1)
+
+ local pat = "abc%=this_text_should_be_gone%=def"
+ local expected_width = 7
+ local width = build_stl_str_hl(pat, expected_width)
+
+ eq(width, expected_width)
+ eq("abc<def", get_str(output_buffer, expected_width))
+ end)
@ZyX-I
ZyX-I Apr 13, 2016 Contributor

It is also good idea to test with multibyte characters to make sure that statusline function knows to align according to display cells occupied and not according to byte length.

@tjdevries
tjdevries Apr 13, 2016 Contributor

I'm not sure exactly how to do that.

Also, I'm not sure how well supported that is currently. While reading through the code, I saw this:

https://github.com/neovim/neovim/blob/master/src/nvim/buffer.c#L2914

@ZyX-I
ZyX-I Apr 13, 2016 Contributor

@tjdevries “fill” characters are characters used in place of spaces in some cases. See :h 'fillchars'.

BTW, you reminded that tests also need to check that statusline may use different characters to fill spacing between sections.

@ZyX-I
ZyX-I Apr 13, 2016 Contributor

@tjdevries And about “how well”: do you know about such plugins as powerline and airline? They use Unicode code points that are not supported by absolutely all existing single-byte encodings: because they are in private use area.

@ZyX-I ZyX-I commented on an outdated diff Apr 13, 2016
test/unit/buffer_spec.lua
+ local expected_width = 20
+ local width = build_stl_str_hl("neo%=vim", expected_width)
+
+ eq(width, expected_width)
+ eq("neo vim", get_str(output_buffer, expected_width))
+ end)
+
+ it('should approximately center text when using %=text%=', function()
+ buffer.setfname(globals.curbuf, to_cstr("test/unit/buffer_spec.lua"), NULL, 1)
+
+ local expected_width = 20
+ local width = build_stl_str_hl("abc%=neovim%=def", expected_width)
+
+ eq(width, expected_width)
+ eq("abc neovim def", get_str(output_buffer, expected_width))
+ end)
@ZyX-I
ZyX-I Apr 13, 2016 Contributor

Can you also repeat with expected_width = 21 (i.e. so that spaces cannot be possibly distributed equally)? I also still do not see tests like foobar%=test%=baz (non-equal width on sides), foobar%=test%=/%=test%=baz (empty side), %=test%= (empty sides).

@ZyX-I ZyX-I commented on an outdated diff Apr 13, 2016
test/unit/buffer_spec.lua
eq(1, width)
eq("1", helpers.ffi.string(output_buffer, width))
end)
+
+ it('should right align when using =', function()
+ buffer.setfname(globals.curbuf, to_cstr("test/unit/buffer_spec.lua"), NULL, 1)
+
+ local expected_width = 20
+ local width = build_stl_str_hl("neo%=vim", expected_width)
+
+ eq(width, expected_width)
+ eq("neo vim", get_str(output_buffer, expected_width))
+ end)
+
+ it('should approximately center text when using %=text%=', function()
@ZyX-I
ZyX-I Apr 13, 2016 Contributor

Based on the fact that tests are nearly identical I would suggest to write all of them like this:

    local peqtest = function(name, width, stl, expected_stl)
      it(name, function()
        buffer.setfname(globals.curbuf, to_cstr("test/unit/buffer_spec.lua"), NULL, 1)

        local expected_width = width
        local actual_width = build_stl_str_hl(stl, expected_width)

        eq(actual_width, expected_width)
        eq(expected_stl, get_str(output_buffer, expected_width))
      end)
    end

    peqtest('should approximately center text when using %=text%=', 20, 'abc%=neovim%=def',
            'abc    neovim    def')

: i.e. use function to generate tests in place of copy-pasting test function over and over.

@ZyX-I ZyX-I and 1 other commented on an outdated diff Apr 13, 2016
test/unit/buffer_spec.lua
+ statusline_test('should, when possible, center text when using %=text%=', 20, 20,
+ 'abc%=neovim%=def', 'abc neovim def')
+ statusline_test('should completely fill the buffer when using %=text%=', 20, 20,
+ 'abc%=neo_vim%=def', 'abc neo_vim def')
+ statusline_test('should have equal spaces even with non-equal sides when using =', 20, 20,
+ 'foobar%=test%=baz', 'foobar test baz')
+ statusline_test('should have equal spaces even with longer right side when using =', 20, 20,
+ 'a%=test%=longtext', 'a test longtext')
+ statusline_test('should handle an empty left side when using ==', 20, 20,
+ '%=test%=baz', ' test baz')
+ statusline_test('should handle an empty right side when using ==', 20, 20,
+ 'foobar%=test%=', 'foobar test ')
+
+ statusline_test('should approximately center text when using %=text%=', 21, 21,
+ 'abc%=neovim%=def', 'abc neovim def')
+ statusline_test('should completely fill the buffer when using %=text%=', 21, 21,
@ZyX-I
ZyX-I Apr 13, 2016 Contributor

Test name is rather strange: buffer is always filled completely, is not it?

(Also still where is %=text%= test (no text on both sides).) And I can suggest

  • %=%=text%= (three empty sections: test regarding how %=%= is handled: situation may appear when some of the center sections appears empty).
  • %=text, text%=
  • %= and just empty string: such edge cases are more likely to provoke a crash.
  • ('%='):rep( --[[STL_MAX_ITEM]] 80) (probably use local STL_MAX_ITEM = 80)
  • ('%='):rep( --[[STL_MAX_ITEM + 1]] 81) (so here it would be STL_MAX_ITEM + 1)
@tjdevries
tjdevries Apr 13, 2016 Contributor

(Deleted my previous comment after I realized I misinterpreted what you were saying).

I will change the wording on the first test and add in those other tests as well. Thank you.

@tjdevries
Contributor
tjdevries commented Apr 18, 2016 edited

@ZyX-I, I was trying to make some tests for multibyte characters and I'm having some confusion here.

For example, if I add two tests like this:

    statusline_test('should handle multibye characters', 10, 10,
      '⌘%=x', '⌘        x')
    statusline_test('should handle multibye characters', 10, 10,
      'Ĉ%=x', 'Ĉ         x')

These tests both fail like so:

Failure → test/unit/buffer_spec.lua @ 273
buffer functions build_stl_str_hl should handle multibye characters
test/unit/buffer_spec.lua:279: Expected objects to be the same.
Passed in:
(string) '⌘       '
Expected:
(string) '⌘         x'

stack traceback:
    test/unit/buffer_spec.lua:279: in function <test/unit/buffer_spec.lua:273>


Failure → test/unit/buffer_spec.lua @ 273
buffer functions build_stl_str_hl should handle multibye characters
test/unit/buffer_spec.lua:279: Expected objects to be the same.
Passed in:
(string) 'Ĉ        '
Expected:
(string) 'Ĉ         x'

However, when I run the Neovim build that I built to test these with, the statusline shows up as expected. I believe this has to do with the expected width of the test. So this is where my question lies.

Should I say that the width is 10, because the width is in terms of "cells"?

If I do so, I believe I will be able to make it work so that the tests pass. I'm just not sure exactly how to handle this idea.

Essentially what happens now is it takes up the width of the screen correctly, but I think it is hard to show in the unittests.

This is what it looks like when it passes:

    local mbyte_statusline_test = function(name, input_width, expected_width, stl, expected_stl)
      it(name, function()
        buffer.setfname(globals.curbuf, to_cstr("test/unit/buffer_spec.lua"), NULL, 1)

        local actual_width = build_stl_str_hl{ pat=stl, maxwidth=input_width }

        eq(expected_stl, get_str(output_buffer, expected_width))
      end)
    end
    mbyte_statusline_test('should handle multibye characters', 10, 11,
      'Ĉ%=x', 'Ĉ        x')
@ZyX-I ZyX-I commented on an outdated diff Apr 18, 2016
test/unit/buffer_spec.lua
- NULL,
- NULL)
- end
-
- it('should copy plain text', function()
- local width = build_stl_str_hl("this is a test")
-
- eq(14, width)
- eq("this is a test", helpers.ffi.string(output_buffer, width))
-
+ before_each(function()
+ buffer_width = 100
+ output_buffer = to_cstr(string.rep(" ", buffer_width))
+ -- Currently assumes fillchar = ' '
+
+ build_stl_str_hl = function (arg)
@ZyX-I
ZyX-I Apr 18, 2016 Contributor

Why it is not local?

@ZyX-I ZyX-I commented on an outdated diff Apr 18, 2016
test/unit/buffer_spec.lua
- end
-
- it('should copy plain text', function()
- local width = build_stl_str_hl("this is a test")
-
- eq(14, width)
- eq("this is a test", helpers.ffi.string(output_buffer, width))
-
+ before_each(function()
+ buffer_width = 100
+ output_buffer = to_cstr(string.rep(" ", buffer_width))
+ -- Currently assumes fillchar = ' '
+
+ build_stl_str_hl = function (arg)
+ if not arg.fillchar then
+ arg.fillchar = 0x20
@ZyX-I
ZyX-I Apr 18, 2016 Contributor

In such cases better write (' '):byte() then some magic number.

@ZyX-I ZyX-I commented on an outdated diff Apr 18, 2016
test/unit/buffer_spec.lua
assert.is_true(8 < width)
neq(NULL, string.find(helpers.ffi.string(output_buffer, width), "Makefile"))
end)
- it('should print the tail file name', function()
- buffer.setfname(globals.curbuf, to_cstr("src/nvim/buffer.c"), NULL, 1)
-
- local width = build_stl_str_hl("%t")
-
- eq(8, width)
- eq("buffer.c", helpers.ffi.string(output_buffer, width))
+ it('should correctly input different fillchars', function()
+ local actual_width = build_stl_str_hl{ pat='abcde%=', maxwidth=10, fillchar=0x21 }
@ZyX-I
ZyX-I Apr 18, 2016 Contributor

Same here: ('!'):byte().

@ZyX-I ZyX-I commented on an outdated diff Apr 18, 2016
test/unit/buffer_spec.lua
- to_cstr(pat),
- false,
- 32,
- 80,
- NULL,
- NULL)
- end
-
- it('should copy plain text', function()
- local width = build_stl_str_hl("this is a test")
-
- eq(14, width)
- eq("this is a test", helpers.ffi.string(output_buffer, width))
-
+ before_each(function()
+ buffer_width = 100
@ZyX-I
ZyX-I Apr 18, 2016 Contributor

Why this is in before_each at all, not to mention it is not local? Even if you must define it here for some reason, you should declare it local in the outer scope (i.e. just before before_each).

@ZyX-I ZyX-I commented on an outdated diff Apr 18, 2016
test/unit/buffer_spec.lua
- eq(14, width)
- eq("this is a test", helpers.ffi.string(output_buffer, width))
-
+ before_each(function()
+ buffer_width = 100
+ output_buffer = to_cstr(string.rep(" ", buffer_width))
+ -- Currently assumes fillchar = ' '
+
+ build_stl_str_hl = function (arg)
+ if not arg.fillchar then
+ arg.fillchar = 0x20
+ end
+
+ return buffer.build_stl_str_hl(globals.curwin,
+ output_buffer,
+ buffer_width,
@ZyX-I
ZyX-I Apr 18, 2016 Contributor

This should not be named “width”, it is length of the buffer in bytes. Width is for screen cells.

@ZyX-I ZyX-I commented on an outdated diff Apr 18, 2016
test/unit/buffer_spec.lua
- it('should print the current line number in the buffer', function()
- buffer.setfname(globals.curbuf, to_cstr("test/unit/buffer_spec.lua"), NULL, 1)
-
- local width = build_stl_str_hl("%l")
-
- eq(1, width)
- eq("0", helpers.ffi.string(output_buffer, width))
-
- end)
-
- it('should print the number of lines in the buffer', function()
- buffer.setfname(globals.curbuf, to_cstr("test/unit/buffer_spec.lua"), NULL, 1)
-
- local width = build_stl_str_hl("%L")
+ eq(actual_width, expected_width)
+ eq(expected_stl, get_str(output_buffer, expected_width))
@ZyX-I
ZyX-I Apr 18, 2016 Contributor

This is not correct. AFAIK output_buffer is NUL-terminated, so expected_width (which is width in cells, not byte length) is not needed here at all.

@ZyX-I
ZyX-I Apr 18, 2016 Contributor

This should fix multibyte tests.

@ZyX-I
ZyX-I Apr 18, 2016 Contributor

More, this makes test actually incorrect: if output_buffer happened to miss NUL or contain it in invalid location (i.e. after the position it should be located in) this bug will not be caught by the test.

@tjdevries
Contributor

@ZyX-I, I have implemented a lot of the things you pointed out in your comments. However, I'm wondering a few things about multi-byte characters now.

This is from the documentation for build_stl_str_hl:

/// @param wp The window to build a statusline for
/// @param out The output buffer to write the statusline to
///            Note: This should not be NameBuff
/// @param outlen The length of the output buffer
/// @param fmt The statusline format string
/// @param use_sandbox Use a sandboxed environment when evaluating fmt
/// @param fillchar Character to use when filling empty space in the statusline
/// @param maxwidth The maximum width to make the statusline
/// @param hltab HL attributes (can be NULL)
/// @param tabtab Tab clicks definition (can be NULL).
  1. For outlen, it says the length of the output buffer. Is that the length in cells or bytes?
  2. For maxwidth, (which is why I had named it maxwidth, expected_width, etc. in the test), is that in cells or bytes?

I will update the documentation to make sure this is clear, because for me it is not clear which should be which when considering multi-byte characters.

Also, are you saying that we shouldn't check eq(actual_width, expected_width)? This was in the original unit tests for the buffer_spec, so I just wanted to make sure that you thought that it should be removed. It is probably not necessary given that we are comparing the whole buffer.

I also don't know that I understand all of Neovim's work with multi-byte characters. For example when I input into neovim, the and whatever character I type afterwards overlap. I'm not sure if that should be happening or how that would affect the statusline. It works fine for other sets of unicode characters, such as Ĉ, so maybe this is just a side issue that doesn't effect me at all, which is fine.

@ZyX-I
Contributor
ZyX-I commented Apr 18, 2016

@tjdevries It is simple and clear: length is always in number of *out elements. In this case sizeof(*out) == 1 i.e. byte. Width always applies to screen cells.

I also don't know that I understand all of Neovim's work with multi-byte characters. For example when I input ⌘ into neovim, the ⌘ and whatever character I type afterwards overlap. I'm not sure if that should be happening or how that would affect the statusline. It works fine for other sets of unicode characters, such as Ĉ, so maybe this is just a side issue that doesn't effect me at all, which is fine.

This has absolutely nothing to do with Neovim. In case some glyph is not present in some font it is taken from another font. But different fonts have different dimentions of one screen cell, usually bigger (“usually” because if it is smaller you will not notice). So use GUI for such characters, usually it draws them saner for some reason.

@tjdevries tjdevries and 1 other commented on an outdated diff Apr 22, 2016
test/unit/buffer_spec.lua
+ -- the format string and the resulting statusline.
+ --
+ -- @param description The description of what the test should be doing
+ -- @param statusline_cell_count The number of cells available in the statusline
+ -- @param expected_cell_count The expected number of cells build_stl_str_hl will return
+ -- @param expected_byte_length The expected byte length of the string
+ -- @param input_stl The format string for the statusline
+ -- @param expected_stl The expected result string for the statusline
+ local function statusline_test (description,
+ statusline_cell_count,
+ expected_cell_count,
+ expected_byte_length,
+ input_stl,
+ expected_stl,
+ arg)
+
@tjdevries
tjdevries Apr 22, 2016 Contributor

@ZyX-I, Is this more in line with what you were thinking for the function that would handle each individual test? I have now removed the before_each and made everything I could find local.

Do you think it is okay to have the optional parameters in the way that I do? I thought this separated out any concerns between tests giving them each the ability to add a new feature to test whenever they wanted, without having to change the old tests.

@ZyX-I
ZyX-I Apr 23, 2016 Contributor

I do not like the idea of the function with seven arguments. While it is easy to understand what most of the arguments mean, meanings of the three numbers in the middle are hard to remember. I also think that expected_byte_length may default to expected_cell_count which may default to statusline_cell_count, so some tests will have one or two arguments less.

@ZyX-I
ZyX-I Apr 23, 2016 Contributor

I mean, understand what arguments mean without looking at the signature.

@tjdevries
tjdevries Apr 23, 2016 Contributor

👍, I like that better as well. I will put that logic in.

@ZyX-I ZyX-I commented on an outdated diff Apr 23, 2016
test/unit/buffer_spec.lua
+ statusline_test('should have equal spaces even with longer right side when using =', 20, 20, 20,
+ 'a%=test%=longtext', 'a test longtext')
+ statusline_test('should handle an empty left side when using ==', 20, 20, 20,
+ '%=test%=baz', ' test baz')
+ statusline_test('should handle an empty right side when using ==', 20, 20, 20,
+ 'foobar%=test%=', 'foobar test ')
+ statusline_test('should handle consecutive empty ==', 20, 20, 20,
+ '%=%=test%=', ' test ')
+ statusline_test('should handle an = alone', 20, 20, 20,
+ '%=', ' ')
+ statusline_test('should right align text when it is alone with =', 20, 20, 20,
+ '%=foo', ' foo')
+ statusline_test('should left align text when it is alone with =', 20, 20, 20,
+ 'foo%=', 'foo ')
+ statusline_test('should handle a much larger amount of = than buffer locations', 20, 20, 20,
+ ('%='):rep(STL_MAX_ITEM), ' ') -- Should be fine, because within limit
@ZyX-I
ZyX-I Apr 23, 2016 Contributor

Just interested: what is it going to do with 'a' .. ('%=a'):rep(STL_MAX_ITEM). Also with different locations of %<. Though I do not see much tests which involve truncation here: only three basic tests and no %<. Especially no multiple %<.

@tjdevries
Contributor

I'm not sure why my test is failing. I've been doing some research on it and it appears to be occurring in describe('buflist_findpat', function(), line 95.

The particular tests that are failing are:

    it('should prefer to match the start of a file path', function()
      local buf1 = buflist_new(path1, buffer.BLN_LISTED)
      local buf2 = buflist_new(path2, buffer.BLN_LISTED)
      local buf3 = buflist_new(path3, buffer.BLN_LISTED)

      eq(buf1.b_fnum, buflist_findpat("test", ONLY_LISTED))
      eq(buf2.b_fnum, buflist_findpat("file", ONLY_LISTED))
      eq(buf3.b_fnum, buflist_findpat("path", ONLY_LISTED))

      close_buffer(NULL, buf1, buffer.DOBUF_WIPE, 0)
      close_buffer(NULL, buf2, buffer.DOBUF_WIPE, 0)
      close_buffer(NULL, buf3, buffer.DOBUF_WIPE, 0)
    end)

    it('should prefer to match the end of a file over the middle', function()
      --{ Given: Two buffers, where 'test' appears in both
      --  And: 'test' appears at the end of buf3 but in the middle of buf2
      local buf2 = buflist_new(path2, buffer.BLN_LISTED)
      local buf3 = buflist_new(path3, buffer.BLN_LISTED)

      -- Then: buf2 is the buffer that is found
      eq(buf2.b_fnum, buflist_findpat("test", ONLY_LISTED))
      --}

      --{ When: We close buf2
      close_buffer(NULL, buf2, buffer.DOBUF_WIPE, 0)

      -- And: Open buf1, which has 'file' in the middle of its name
      local buf1 = buflist_new(path1, buffer.BLN_LISTED)

      -- Then: buf3 is found since 'file' appears at the end of the name
      eq(buf3.b_fnum, buflist_findpat("file", ONLY_LISTED))
      --}

      close_buffer(NULL, buf1, buffer.DOBUF_WIPE, 0)
      close_buffer(NULL, buf3, buffer.DOBUF_WIPE, 0)
    end)

It seems whenever the buflist_findpat is called more than once in a single test, it fails. I cannot figure out why. Does anyone have any suggestions? I'm not really sure how it's related to my code either.

Thanks everyone.

@KillTheMule
Contributor
KillTheMule commented May 23, 2016 edited

I guess you're seeing the same thing I did in #4685, it's not this test, the output has timing issues. Look at the comments to see things to try.

@justinmk
Member

Pics? :)

@tjdevries
Contributor

Sorry @justinmk :) I will try and get some pictures up maybe this week. I just graduated this past week and started a new job, so things have been hectic on my end haha! I will update the status at the top as well, so it's more clear where we're sitting. Currently, I just have some random test failing that doesn't seem related to my stuff at all, so that's the part that I've been trying to figure out. I haven't gone through all the steps that KillTheMule mentioned earlier.

Other than that, it seems pretty much ready!

@tjdevries
Contributor

@justinmk, as requested, I uploaded two pictures :) I can add some more if you'd like. Are there any particular scenarios you would be interested in seeing?

I'm going to try this week to do some of the things that @KillTheMule linked.

@justinmk
Member
justinmk commented Jun 4, 2016

Cool! Was hoping to get this in 0.1.5 but don't want to rush it.

@tjdevries
Contributor

@justinmk, it appears that all the tests have passed besides the lint (which I just made a quick PR to fix). I think this is pretty much ready, unless you can think of anything else.

I can do a rebase over these commits so that the history is a little nicer if you prefer.

@mhinz mhinz commented on an outdated diff Jun 9, 2016
src/nvim/buffer.c
+
+ if (middle_mark > 0) {
+ // Handle a combination of middle mark and centering
+ //
+ // text\ %[this\ text\ here%=\ and\ some%]finishing
+ // text his text here and some finishing
+ // quad1 quad2|quad3 quad4
+ // --fill1-- --fill2--
+ // int len_a = item[center_begin].start - out;
+ int len_b = item[middle_mark].start - out;
+ // int len_c = item[center_end].start - out;
+ // Not sure about this one
+ int len_d = width;
+
+ int fill_left = maxwidth / 2 - len_b;
+ int fill_right = maxwidth - maxwidth / 2 - len_d + len_b;
@mhinz
mhinz Jun 9, 2016 Member

Personally I'd always be explicit in such situations or at least use an visual style to make the operator precedence clearer, so a - (b / c) - d + e or a - b/c - d +e. This case is simple enough and doesn't need to be changed, but for the future it might be nice. ;-)

@mhinz mhinz commented on an outdated diff Jun 9, 2016
src/nvim/buffer.c
- // Fill the middle section with our fill character
- for (char_u *s = item[item_idx].start; s < middle_end; s++)
- *s = '=';
+ int item_idx = center_begin;
+ // Adjust the offset of any items after the middle
+ for (item_idx++; item_idx <= center_end; item_idx++)
@mhinz
mhinz Jun 9, 2016 edited Member

This is a very uncommon style and might confuse reviewers about when exactly item_idx is incremented. Please change it to:

item_idx++;
for (; item_idx <= center_end; item_idx++)

...or simply with the line before...

int item_idx = center_begin + 1;
for (; item_idx <= center_end; item_idx++)

Oh, and you forgot the parentheses for the if. ;-)

@justinmk
Member

Anyone have any suggestions about documentation? Where else should this be mentioned?

Is there a maximum number of separators? The doc implies unlimited.

If there are no other comments I'll merge after verifying the tests.

@tjdevries
Contributor

Documentation: I'm not sure where else to mention this, but I found all the help places I could.

Max Separators: There is a maximum number of separators, which is limited by STL_MAX_ITEM (defined in nvim/vim.h). However, that's the limit for the number of any amount of items in your statusline. Otherwise it does not limit it.

@KillTheMule KillTheMule commented on an outdated diff Jun 13, 2016
test/unit/buffer_spec.lua
+ -- @param statusline_cell_count The number of cells available in the statusline
+ -- @param input_stl The format string for the statusline
+ -- @param expected_stl The expected result string for the statusline
+ --
+ -- Options can be placed in an optional dictionary as the last parameter
+ -- @option
+ -- @option expected_cell_count The expected number of cells build_stl_str_hl will return
+ -- @option expected_byte_length The expected byte length of the string
+ local function statusline_test (description,
+ statusline_cell_count,
+ input_stl,
+ expected_stl,
+ arg)
+
+ -- arg is our optional parameter
+ arg = arg or {}
@KillTheMule
KillTheMule Jun 13, 2016 edited Contributor

This needs a local.

(e) You should rebase on master, so the testlinting is added.

@KillTheMule KillTheMule commented on an outdated diff Jun 13, 2016
test/unit/buffer_spec.lua
+ -- @option expected_byte_length The expected byte length of the string
+ local function statusline_test (description,
+ statusline_cell_count,
+ input_stl,
+ expected_stl,
+ arg)
+
+ -- arg is our optional parameter
+ arg = arg or {}
+
+ local fillchar = arg.fillchar or (' '):byte()
+ local expected_cell_count = arg.expected_cell_count or statusline_cell_count
+ local expected_byte_length = arg.expected_byte_length or expected_cell_count
+
+ it(description, function()
+ if arg.file_name then
@KillTheMule
KillTheMule Jun 13, 2016 Contributor

It should be mentioned in the function comment that arg can have a key file_name.

@justinmk
Member
justinmk commented Jun 13, 2016 edited

STL_MAX_ITEM

Ok so basically unlimited for practical purposes, not worth mentioning in the user manual.

I can't seem to figure out how to remove mentions that are generated in auto.

@tjdevries Is this still an issue? If so I can look into it.

@KillTheMule KillTheMule commented on an outdated diff Jun 13, 2016
test/unit/buffer_spec.lua
+ -- @param expected_stl The expected result string for the statusline
+ --
+ -- Options can be placed in an optional dictionary as the last parameter
+ -- @option
+ -- @option expected_cell_count The expected number of cells build_stl_str_hl will return
+ -- @option expected_byte_length The expected byte length of the string
+ local function statusline_test (description,
+ statusline_cell_count,
+ input_stl,
+ expected_stl,
+ arg)
+
+ -- arg is our optional parameter
+ arg = arg or {}
+
+ local fillchar = arg.fillchar or (' '):byte()
@KillTheMule
KillTheMule Jun 13, 2016 Contributor

It should be mentioned in the function comment that arg can have a key fillchar.

@tjdevries
Contributor

@justinmk:

STL_MAX_ITEM

Yeah, unlimited for user purposes.

mentions in auto

Apparently they have gone away. I just grepped for the old term, and it's not there anymore. So someone must have fixed that issue, or it was just left over from an old build of mine or something.

@KillTheMule:

I added the two optional parameters to the documentation of the function, and also rebased on master. I didn't find any linting errors with files that I have touched (there was some new linting errors, but they were in files that were not changed in this PR).

As a side note, this means my build will probably fail because I'll have linting errors from other files.

@mhinz mhinz commented on an outdated diff Jun 13, 2016
src/nvim/buffer.c
// fill up the available space.
} else if (width < maxwidth
- && STRLEN(out) + maxwidth - width + 1 < outlen) {
- for (int item_idx = 0; item_idx < itemcnt; item_idx++) {
- if (item[item_idx].type == Middle) {
- // Move the statusline to make room for the middle characters
- char_u *middle_end = item[item_idx].start + (maxwidth - width);
- STRMOVE(middle_end, item[item_idx].start);
-
- // Fill the middle section with our fill character
- for (char_u *s = item[item_idx].start; s < middle_end; s++)
- *s = fillchar;
+ && STRLEN(out) + maxwidth - width + 1 < outlen) {
+ // Find how many separators there are, which we will use when
+ // figuring out how many groups there are.
@mhinz
mhinz Jun 13, 2016 Member

Nitpick: superfluous space

@mhinz mhinz commented on an outdated diff Jun 13, 2016
src/nvim/buffer.c
- for (char_u *s = item[item_idx].start; s < middle_end; s++)
- *s = fillchar;
+ && STRLEN(out) + maxwidth - width + 1 < outlen) {
+ // Find how many separators there are, which we will use when
+ // figuring out how many groups there are.
+ int num_separators = 0;
+ for (int i = 0; i < itemcnt; i++) {
+ if (item[i].type == Separate) {
+ num_separators++;
+ }
+ }
+
+ // If we have separated groups, then we deal with it now
+ if (num_separators) {
+ // Create an array of the start location for each
+ // separator mark.
@mhinz
mhinz Jun 13, 2016 Member

Space.

@mhinz mhinz and 1 other commented on an outdated diff Jun 13, 2016
src/nvim/buffer.c
- // Adjust the offset of any items after the middle
- for (item_idx++; item_idx < itemcnt; item_idx++)
- item[item_idx].start += maxwidth - width;
+ int standard_spaces = (maxwidth - width) / num_separators;
+ int final_spaces = (maxwidth - width) -
+ standard_spaces * (num_separators - 1);
+
+ char_u *sep_loc;
+ int dislocation;
+ int item_idx;
@mhinz
mhinz Jun 13, 2016 Member

Style: I'd move the above 3 declarations into the loop below and initialize them right away. Compilers will generate exactly the same code nowdays and it looks cleaner.

@tjdevries
tjdevries Jun 13, 2016 edited Contributor

I'm not sure which you're saying, I can change it to either one that is the preferred way.

for (int i=0...) {
    char_u *sep_loc;
    ....
}

Or

for (int i=0...) {
    char_u *sep_loc = item[separator_locations[i]].start + dislocation;
    ....
}
@tjdevries
tjdevries Jun 13, 2016 Contributor

I did the first way. Just pushed up the new version.

@mhinz
mhinz Jun 13, 2016 edited Member

Sorry, but I meant the latter version actually. :-) There's simply no use of using declarations if they get initialized a few lines later anyway. Basically, don't declare variables until they're really needed.

@mhinz mhinz and 2 others commented on an outdated diff Jun 13, 2016
test/unit/buffer_spec.lua
+ -- @param input_stl The format string for the statusline
+ -- @param expected_stl The expected result string for the statusline
+ --
+ -- @param arg Options can be placed in an optional dictionary as the last parameter
+ -- .expected_cell_count The expected number of cells build_stl_str_hl will return
+ -- .expected_byte_length The expected byte length of the string
+ -- .file_name The name of the file to be tested (useful in %f type tests)
+ -- .fillchar The character that will be used to fill any 'extra' space in the stl
+ local function statusline_test (description,
+ statusline_cell_count,
+ input_stl,
+ expected_stl,
+ arg)
+
+ -- arg is our optional parameter
+ local arg = arg or {}
@mhinz
mhinz Jun 13, 2016 Member

lualint will complain about the local here, because arg is already defined (since it's a function parameter). Just remove the local.

@justinmk
justinmk Jun 13, 2016 Member

Better to use a different name (with local)--generally not good practice to re-assign arguments.

@tjdevries
tjdevries Jun 13, 2016 Contributor

I changed it to local option and gave a brief explanation.

@mhinz
Member
mhinz commented Jun 13, 2016

LGTM, apart from the small nitpicks.

@justinmk justinmk changed the title from [RFC] Center Items in the statusline / tagline to [RFC] statusline: unlimited alignment sections Jun 13, 2016
@mhinz mhinz and 1 other commented on an outdated diff Jun 13, 2016
src/nvim/buffer.c
+
+ for (int i = 0; i < num_separators; i++) {
+ char_u *sep_loc;
+ int dislocation;
+ int item_idx;
+
+ dislocation = (i == (num_separators - 1)) ?
+ final_spaces : standard_spaces;
+ sep_loc = item[separator_locations[i]].start + dislocation;
+ STRMOVE(sep_loc, item[separator_locations[i]].start);
+ for (char_u *s = item[separator_locations[i]].start; s < sep_loc; s++) {
+ *s = fillchar;
+ }
+
+ item_idx = separator_locations[i] + 1;
+ for (; item_idx < itemcnt; item_idx++) {
@mhinz
mhinz Jun 13, 2016 edited Member

This could be one line:

for (int item_idx = separator_locations[i] + 1; item_idx < itemcnt; item_idx++) {

But for clarity purposes, this work just as well for me:

int item_idx = separator_locations[i] + 1;
for (; item_idx < itemcnt; item_idx++)
@tjdevries
tjdevries Jun 13, 2016 Contributor

I prefer it in the two lines tbh. I can change it though if that's desired.

@tjdevries
Contributor

@justinmk, I think I've answered @mhinz suggestions. Is there anything else you'd like me to do here?

@mhinz
Member
mhinz commented Jun 13, 2016

Functionally everything is fine, so it can be merged now unless someone else is having any remarks.

@justinmk
Member

I'd like to verify the tests locally, unless someone else does so.

@mhinz
Member
mhinz commented Jun 13, 2016 edited

make, make test and make testlint work without problems.

@tjdevries
Contributor

@mhinz, just fixed the two cases that you mentioned. I did not see the comment that you had about choosing the opposite of what I ended up doing. :) So those should be fixed now.

@mhinz mhinz changed the title from [RFC] statusline: unlimited alignment sections to [RDY] statusline: unlimited alignment sections Jun 14, 2016
@mhinz
Member
mhinz commented Jun 14, 2016

@tjdevries No worries, it was in a mergeable state before these cosmetic changes already, but thanks for doing it anyways. :-)

Usually I'd merge this by now, but I'm not sure what kind of local tests @justinmk had in mind, so I'll wait for him.

@marvim marvim added RDY and removed RFC labels Jun 14, 2016
@justinmk
Member
justinmk commented Jun 14, 2016 edited

To verify the tests, one needs to change the C logic and see if the tests fail correctly. Is that what you did @mhinz ?

@mhinz
Member
mhinz commented Jun 14, 2016 edited

I did now. ;]

I inverted operators in the for loops and sabotaged the calculations and the tests failed just fine.

@justinmk justinmk and 1 other commented on an outdated diff Jun 14, 2016
test/unit/buffer_spec.lua
@@ -211,93 +210,246 @@ describe('buffer functions', function()
end)
describe('build_stl_str_hl', function()
+ local buffer_byte_size = 100
+ local STL_MAX_ITEM = 80
+ local output_buffer = ''
+
+ -- This function builds the statusline
+ --
+ -- @param arg Optional arguments are:
+ -- .pat The statusline format string
+ -- .fillchar The fill character used in the statusline
+ -- .maximum_cell_count The number of cells available in the statusline
+ local build_stl_str_hl = function (arg)
@justinmk
justinmk Jun 14, 2016 edited Member

Declaring a function this way is not recommended, see mpeterv/luacheck#60. Instead:

local function build_stl_str_hl(arg)

Though the luacheck issue doesn't apply here, it's better that we use the same form everywhere.

@tjdevries
tjdevries Jun 14, 2016 Contributor

Fixed (I think :) )

@tjdevries @tjdevries tjdevries Add new functionality to the `=` marker in the STL
This new functionality is explained in the documentation.

Also, many tests have been added to the buffer_spec.lua file
63ec2a6
@justinmk
Member

:shipit:

@mhinz
Member
mhinz commented Jun 14, 2016 edited

Merged. Good work, @tjdevries!

(I cherry-picked the commit, since it wasn't rebased. :-)

@mhinz mhinz closed this Jun 14, 2016
@jszakmeister jszakmeister removed the RDY label Jun 14, 2016
@KillTheMule KillTheMule referenced this pull request in neovim/neovim.github.io Jul 11, 2016
Merged

Newssssssssss #136

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment