New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Port vim's breakindent patch to neovim's codebase. (vim patches 7.4.338, 7.4.346, 7.4.352, 7.4.353, 7.4.370, 7.4.371, 7.4.388) #691

Merged
merged 8 commits into from Aug 20, 2014

Conversation

Projects
None yet
8 participants
@fmoralesc
Copy link
Contributor

fmoralesc commented May 6, 2014

Using vim's patches 7.4.338, 7.4.346, 7.4.352, 7.4.353, 7.4.370, 7.4.371, 7.4.388 by Christian Brabandt.

Fixes issue #501.

@fmoralesc fmoralesc changed the title Port vim's breakindent patch to neovim's codebase. [RFC] Port vim's breakindent patch to neovim's codebase. May 6, 2014

@fmoralesc fmoralesc changed the title [RFC] Port vim's breakindent patch to neovim's codebase. [WIP] Port vim's breakindent patch to neovim's codebase. May 6, 2014

@aktau

This comment has been minimized.

Copy link
Member

aktau commented May 6, 2014

I think it would be cool to have this included in neovim if the community can agree on it. However it appears to still need some fixing. Clint reports some formatting errors (you can run that manually on your own PC, no need to wait for travis), and I think clang is catching some type conversion bug:

/home/travis/build/neovim/neovim/src/screen.c:2874:17: error: expression which
      evaluates to zero treated as a null pointer constant of type 'char_u *'
      (aka 'unsigned char *') [-Werror,-Wnon-literal-null-conversion]
             p_extra = NUL;
                       ^~~
/home/travis/build/neovim/neovim/src/ascii.h:25:25: note: expanded from macro
      'NUL'
#define NUL             '\000'
                        ^~~~~~
1 error generated.

I thought we had even removed the NUL macro... It appears not.

@philix

This comment has been minimized.

Copy link
Member

philix commented May 6, 2014

@aktau the change that removed NUL was reverted for good. And NULL should
be used for pointers instead.
On May 6, 2014 10:03 AM, "Nicolas Hillegeer" notifications@github.com
wrote:

I think it would be cool to have this included in neovim if the community
can agree on it. However it appears to still need some fixing. Clint
reports some formatting errors (you can run that manually on your own PC,
no need to wait for travis), and I think clang is catching some type
conversion bug:

/home/travis/build/neovim/neovim/src/screen.c:2874:17: error: expression which
evaluates to zero treated as a null pointer constant of type 'char_u *'
(aka 'unsigned char *') [-Werror,-Wnon-literal-null-conversion]
p_extra = NUL;
^~~/home/travis/build/neovim/neovim/src/ascii.h:25:25: note: expanded from macro
'NUL'#define NUL '\000'
^~~~~~1 error generated.

I thought we had even removed the NUL macro... It appears not.


Reply to this email directly or view it on GitHubhttps://github.com//pull/691#issuecomment-42298867
.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented May 6, 2014

@aktau the change that removed NUL was reverted for good. And NULL should
be used for pointers instead.

Of course NULL should be used for pointers. I wasn't aware that removing NUL was reverted though. At any rate, the patch should get a good neovim style treatment. But it sounds interesting.

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented May 6, 2014

Yes, I simply reapplied the changes in the original patch and in the places the changes had side-effects (regex-nfa.c, for example, which was not patched). I think the clang error @aktau mentions is the main issue with this, besides style formatting. So you suggest I replace NUL by NULL in this instance? That was my hunch, but wasn't sure.

Anyway, as far as I tested, the changes are working fine, and 'breakindent' is a very nice feature to have (as maintainer of the pandoc integration plugin for vim, I would want to have it).

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented May 6, 2014

Thanks @fmoralesc. This is also the first bounty patch, so we will be setting precedent with this patch. I definitely don't want to have onerous requirements for bounties, but I do think this patch deserves some unit tests, and in general, bounty patches should include:

We also need to decide how far we want to go with new features before @tarruda and ZyX reach their milestones. If someone posts a sweet patch for multicursor before we have msgpack API ready, I would hesitate to include it right now.

breakindent seems "manageable enough", but it does introduce non-zero entropy at a perhaps delicate time. If I recall, there are some subtle bugs with it (which is why there are mutliple versions of the patch floating around). Those would need to be nailed down now rather than later.

@fmoralesc None of this is directed at you and your PR is very appreciated. I hope that goes without saying.

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented May 6, 2014

@justinmk I completely understand, and agree. Indeed, I thought about the issues last night when I was porting the patch because it touches eval.c, which is a rather hairy point right now because of ZyX's work on the transpiler. I simply started work on this because it seemed simple enough and I myself want it, and proposed the pull request because it worked. ;). I'll try to fix at least the above mentioned issues with it (as you mention, there are also problems with the code itself, and I'm not sure right now what would be the better way of fixing them), but if you or the community think it's best to leave it aside for the moment, that's fine for me (and if someone else further down the road comes up with a better implementation, I would rather have them gather the bounty.)

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented May 8, 2014

Christian just posted an updated patch to vim_dev. Not sure if it will get accepted soon, but he knows his way around vim source, so it should be quality. If @fmoralesc can push this forward, I consider the bounty satisfied.

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented May 8, 2014

I'll check that out.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented May 8, 2014

Here's the vim_dev thread: https://groups.google.com/forum/#!topic/vim_dev/ZGIQQPtVhkI

Might want to wait a couple days to see if Ken Takata's patch is merged with Christian's.

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented May 8, 2014

I've merged Christian's changes into my branch. Some issues with the version I ported are fixed in his, but not all.

I'll start checking what differences Ken Takata's patch might have. Also, I would want some input as to whether to include Chirstian's sbr_linestart_breakindent patch too. I believe it is a good change, but it is not essential.

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented May 8, 2014

OK. Only substantive difference between them is in eval.c. Christian's patch has

  if (argvars[2].v_type != VAR_UNKNOWN)
    lnum = get_tv_number(&argvars[2]);

where Ken's uses get_tv_lnum instead of get_tv_number. I think this makes Ken's version of strdisplaywidth() API more flexible, since it allows the use of the expressions available for line(), like "." or "$".

Overall, Christians patch seems more up-to-date.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented May 12, 2014

Christian is still updating the patch:

https://groups.google.com/d/msg/vim_dev/ZGIQQPtVhkI/1KyiwM21nLcJ

I'd wait another week or so, then we can think about merging this.

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented May 12, 2014

Yes, I noticed. That's why I didn't post any news this weekend.

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented May 19, 2014

I'll postpone further work (for now, it seems chrisbra has stopped updating the patch though) until the header generation stuff is merged into master. I was also thinking of cleaning up my master branch, and opening a new pull request, so if merged, it is cleaner.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented May 19, 2014

@fmoralesc Thank you! Sounds good, but just force-push to this PR. No need to create a new PR.

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented May 19, 2014

Will do.

@justinmk justinmk added the bounty label May 24, 2014

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Jun 26, 2014

The breakindent patch has been merged into vim itself, so I'll port that into the current codebase. I'll have to do the bulk of the work tonight, because I'm traveling tomorrow and I won't have internet access for a few days.

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Jun 26, 2014

I've ported the final patch included in vim, and seems to be passing the travis checks. This pull request also includes vim's tests for breakindent. I've also created pull requests in the vimscript and docs repos with the documentation and support file changes included in the patch, so this batch of changes should cover @justinmk's requests above. ;)

@fmoralesc fmoralesc changed the title [WIP] Port vim's breakindent patch to neovim's codebase. [RFC] Port vim's breakindent patch to neovim's codebase. Jun 26, 2014

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Jun 26, 2014

I should mention the final version of the patch doesn't touch eval.c, so it seems safer to include in neovim in regards to the viml->lua stuff.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 26, 2014

Thank you so much!

@roryokane

This comment has been minimized.

Copy link

roryokane commented Jun 27, 2014

Links to the related pull requests in the vimscript and docs repos:

@cseelus

This comment has been minimized.

Copy link

cseelus commented Jul 11, 2014

Yes, thanks @fmoralesc!

@fmoralesc fmoralesc changed the title [RFC] Port vim's breakindent patch to neovim's codebase. [RFC] Port vim's breakindent patch to neovim's codebase. (vim patch 7.4.338) Aug 9, 2014

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Aug 16, 2014

So, I haven't been taking notice of this, and surely the patch must be updated. Does anyone know what is required to merge this into master? I've noticed the runtime and docs files have been reintroduced, so I guess I'll start by updating that.

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Aug 16, 2014

I see the associated changes in the runtime and docs have already been merged. What's pending?

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 16, 2014

@fmoralesc Well, I believe Christian made a few patches since this was last updated:

https://groups.google.com/d/msg/vim_dev/-S-FFqOW-so/Zy8wrShwHWoJ

But we can merge this if you want to just handle those as vim-patches instead?

@fmoralesc fmoralesc force-pushed the fmoralesc:master branch 3 times, most recently from 5a690c8 to edbf889 Aug 20, 2014

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Aug 20, 2014

Done.

char_u *p;
int bri_shift = 0;
long bri_min = 20;
int bri_sbr = FALSE;

This comment has been minimized.

@oni-link

oni-link Aug 20, 2014

Contributor

int should be replaced with bool and FALSE with false.

This comment has been minimized.

@fmoralesc

fmoralesc Aug 20, 2014

Contributor

Done.

else if (STRNCMP(p, "sbr", 3) == 0)
{
p += 3;
bri_sbr = TRUE;

This comment has been minimized.

@oni-link

oni-link Aug 20, 2014

Contributor

If type of bri_sbr is changed to bool this should be true.

@fmoralesc fmoralesc force-pushed the fmoralesc:master branch from edbf889 to 757b387 Aug 20, 2014

fmoralesc added some commits Jun 26, 2014

vim-patch: 7.4.346
Do not cache "brishift".

Indent was not updated when changing 'breakindentopt'.

Reported by itchiny, fixed by chrisbra in vim patch 7.3.346
(https://code.google.com/p/vim/source/detail?r=3248c6e40aee01a7254d111dd846c6ec7889a804).
vim-patch: 7.4.353
Make 'breakindent' work with the 'list' option.

Originally patched in vim patch 7.4.353, by chrisbra
(https://code.google.com/p/vim/source/detail?r=d42a1d3b74d40f580359dbd139d2d0dfa7235252)

Updated version.c.
vim-patch:7.4.371
Problem:    When 'linebreak' is set control characters are not correctly
displayed. (Kimmy Lindvall)
Solution:   Set n_extra. (Christian Brabandt)

@fmoralesc fmoralesc force-pushed the fmoralesc:master branch from 757b387 to 39c161d Aug 20, 2014

//368,
//367,
//366,
//365,

This comment has been minimized.

@Hinidu

Hinidu Aug 20, 2014

Contributor

I think you accidentally removed these patches.

This comment has been minimized.

@fmoralesc

fmoralesc Aug 20, 2014

Contributor

Yeah---

fmoralesc added some commits Aug 16, 2014

vim-patch:7.4.370
Problem:    Linebreak test fails when encoding is not utf-8. (Danek
Duvall)
Solution:   Split the test in a single byte one and a utf-8 one.
(Christian Brabandt)
vim-patch:7.4.388
Problem:    With 'linebreak' set and 'list' unset a Tab is not counted
            properly. (Kent Sibilev)
Solution:   Check the 'list' option. (Christian Brabandt)
vim-patch: 7.4.352
Problem:    With 'linebreak' a tab causes a missing line break.
Solution:   Count a tab for what it's worth also for shorter lines.
            (Christian Brabandt)
update src/nvim/testdir/Makefile
include breakindent and list related tests

@fmoralesc fmoralesc force-pushed the fmoralesc:master branch from 39c161d to 3b0f7fe Aug 20, 2014

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Aug 20, 2014

I think the travis build got bitten by issue #1063 .

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 20, 2014

@fmoralesc Restarted build.

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Aug 20, 2014

Thanks, @justinmk.

justinmk added a commit that referenced this pull request Aug 20, 2014

Merge pull request #691 from fmoralesc/master
Port vim's breakindent patch to neovim's codebase. (vim patches 7.4.338, 7.4.346, 7.4.352, 7.4.353, 7.4.370, 7.4.371, 7.4.388)

@justinmk justinmk merged commit 118a31c into neovim:master Aug 20, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 20, 2014

Thanks again! Will figure out the bounty thing ASAP.

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Aug 20, 2014

No hurries. Thanks to you all, actually. Feels nice to have this merged.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 26, 2014

@fmoralesc Not sure if you know already, but I just read the bountysource FAQ:

https://www.bountysource.com/faq#bounties

and it says you'll need to contact them to claim the bounty. It seems a bit inconvenient, actually. There's nothing I can do on my end, sorry.

I know you probably don't care, but since this is setting a precedent, I wanted to follow up :)

@fmoralesc

This comment has been minimized.

Copy link
Contributor

fmoralesc commented Aug 26, 2014

Sure ;) I believe you guys must close issue #501 in order for this to proceed:

https://www.bountysource.com/faq#what-happens-after-i-post-a-bounty

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 26, 2014

Done

@splinterofchaos splinterofchaos referenced this pull request Sep 13, 2014

Merged

[RFC] 7.4.371. #1165

dwb pushed a commit to dwb/neovim that referenced this pull request Feb 21, 2017

Merge pull request neovim#691 from neomake/vimhelplint-through-docker
Travis: run vimhelplint through docker_vimhelplint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment