Skip to content
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

vimacs: init at 2016-03-24 #69130

Merged
merged 2 commits into from Mar 27, 2020
Merged

vimacs: init at 2016-03-24 #69130

merged 2 commits into from Mar 27, 2020

Conversation

millerjason
Copy link
Contributor

modified:   maintainers/maintainer-list.nix
new file:   pkgs/applications/editors/vim/vimacs.nix
modified:   pkgs/misc/vim-plugins/generated.nix
modified:   pkgs/misc/vim-plugins/overrides.nix
modified:   pkgs/top-level/all-packages.nix
Motivation for this change

vimacs is a vim-based editor with emacs key bindings

It is both a plugin as well as a wrapper around vim. The wrapper is needed to set up vim as a modeless editor.

For the nix derivation, I've added the RTP and overrides to the wrapper commands allowing users to find this usable without any complex customization in vimrc. This should let it co-exist happily with the original vim they are using.

I've tested vimacs (vm) and gvimacs (gvm) on both darwin and Linux built with --pure.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@chkno
Copy link
Member

chkno commented Oct 24, 2019

Please add andrep/vimacs to vim-plugin-names. This is what causes it to appear in generated.nix.

Why add set nopaste and inoremap <C-c> <C-l>? (What is it about nix/nixpkgs that requires this here instead of either upstream on in users' configs?)

I was able to build and use this on NixOS.

Thanks!

@millerjason
Copy link
Contributor Author

millerjason commented Oct 25, 2019

Please add andrep/vimacs to vim-plugin-names. This is what causes it to appear in generated.nix.

Done - added this, rebased against master, and broke out maintainer addition to a separate commit.

Why add set nopaste and inoremap <C-c> <C-l>? (What is it about nix/nixpkgs that requires this here instead of either upstream on in users' configs?)

The documentation generally suggests it is on the user to add such modifications to their vimrc file, although as you might expect this requires the end-user to not just use the application but also make corresponding modifications to their environment. The documentation around this is, at best, confusing. In my experience, I've found this undesirable for novice users expecting "emacs-like" behavior.

One of the great features of Nix is the ability to use RTP to optionally include the module, so you can now use vimacs in both ways -- if someone adds the vimPlugin and uses a standard vim plugin loader, they have access to using the module in a vim-mode and can change their vimrc as desired for any configuration (or partial configuration) of their choosing. If they use the script, they get the emacs-style options turned on without vimrc changes affecting their base vim runs.

I was able to build and use this on NixOS.

Thanks!

Really appreciate you taking a look!

Thanks, Jason

@chkno
Copy link
Member

chkno commented Oct 25, 2019

Which documentation suggests adding these two specific things? I don't see them in http://algorithm.com.au/code/vimacs/about/ or in https://github.com/andrep/vimacs .

I appreciate that this is trying to be helpful, but I'm wary of making vimacs work differently in Nix than elsewhere, and just the difference creating another little pain point when someone is trying to migrate a bunch of their things to Nix at the same time.

Best thing: If these really should just be defaults, let's propose them upstream (such as like this: https://github.com/chkno/vimacs/pull/1/files ). Having links to documentation where this is recommended and being able to say in the pull request "Why say 'should' in documentation instead of just doing it?" would create the right kind of conversation, I think.

Second best thing: 1. Explain in a comment in pkgs/applications/editors/vim/vimacs.nix why this should be done (doc links), why it can't go upstream, and why it's painful to push it off to the user, and 2. Make it easy to turn it off, such as by having buildCommand pull in this part from another field that can be overridden:

  extraArgs = ''-c "set nopaste" -c "inoremap <C-c> <C-l>"'';
  buildCommand = ''
    ...
    '--cmd "let g:VM_Enabled = 1" --cmd "set rtp^=@rtp@" ${extraArgs}' \
    ...

and then if someone doesn't want this, they can

self: super: {
  vimacs = super.vimacs.overrideAttrs(_: { extraArgs = ""; });
}

@millerjason
Copy link
Contributor Author

I appreciate that this is trying to be helpful, but I'm wary of making vimacs work differently in Nix than elsewhere, and just the difference creating another little pain point when someone is trying to migrate a bunch of their things to Nix at the same time.

That's sensible, I appreciate your point of view. We can match upstream in both cases and leave it to the user to override.

  extraArgs = ''-c "set nopaste" -c "inoremap <C-c> <C-l>"'';
  buildCommand = ''
    ...
    '--cmd "let g:VM_Enabled = 1" --cmd "set rtp^=@rtp@" ${extraArgs}' \
    ...

and then if someone doesn't want this, they can

self: super: {
  vimacs = super.vimacs.overrideAttrs(_: { extraArgs = ""; });
}

I was planning to do the opposite (leave extraArgs empty and support override to add them).

I'm pretty new to Nix, but tried testing this as suggested above with a .config/nixpkgs/overlays and found that I could override extraArgs, but it would not apply to the buildCommand expansion.

Would extraArgs need to move to arguments to avoid having the end user also override the buildCommand? Or am I not applying it at the right point/way?

@jonringer
Copy link
Contributor

also, for adding vim plugins, you should have one update commit, then another commit where you add the name of the plugin and run update again. https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/vim.section.md#adding-new-plugins-to-nixpkgs

@millerjason
Copy link
Contributor Author

also, for adding vim plugins, you should have one update commit, then another commit where you add the name of the plugin and run update again. https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/vim.section.md#adding-new-plugins-to-nixpkgs

@jonringer updated, resolved merge conflicts and retested on Ubuntu and Catalina. LMK if you need anything additional here.

Thanks

@jonringer
Copy link
Contributor

cc @timokau @evanjs do you mind taking a look?

Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Leaving only technical comments, I haven't heard of the project yet and can't comment on it.

I have a slight worry that this is abandoned on init (2016-03-24, copyright on the website till 2010). It seems a bit like this is a "because I can" project that was mostly done for the novelty. Are you actually using this day-to-day @millerjason?

pkgs/applications/editors/vim/vimacs.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/vim/vimacs.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/vim/vimacs.nix Show resolved Hide resolved
pkgs/applications/editors/vim/vimacs.nix Show resolved Hide resolved
pkgs/misc/vim-plugins/overrides.nix Show resolved Hide resolved
pkgs/misc/vim-plugins/overrides.nix Show resolved Hide resolved
@millerjason
Copy link
Contributor Author

Leaving only technical comments, I haven't heard of the project yet and can't comment on it.

I have a slight worry that this is abandoned on init (2016-03-24, copyright on the website till 2010). It seems a bit like this is a "because I can" project that was mostly done for the novelty. Are you actually using this day-to-day @millerjason?

The original versions date back to 2002, so while the full history isn't in github there have been a number of releases over a decade span (see vim.org for history). By measures of vim karma, github stars, or forks this is above other things recently accepted into Nix. You will find users have filed issues against vim as recently as 2019 (vim/vim#4974) referencing it so I do not consider it abandoned. Emacs key bindings haven't changed in a long time, so I don't expect this module to need constant modifications.

I do use this day-to-day and find it the best way to support emacs key binding in vim. Its complexity to setup outside of Nix does make it hard for novice users to adopt, but Nix helps a lot in that respect.

@timokau
Copy link
Member

timokau commented Mar 20, 2020

After reading http://algorithm.com.au/code/vimacs/about/ I'm not exactly a convert, but I do understand why somebody might want to use it.

What's not entirely clear to me yet is: Why does this need to wrap vim, why isn't the plugin sufficient?

@millerjason
Copy link
Contributor Author

What's not entirely clear to me yet is: Why does this need to wrap vim, why isn't the plugin sufficient?

Changing vim bindings to support modeless editing obviously will override some of the original vim behavior, so the wrapper script allows you to run vim to get classic behavior and vimacs to get modeless behavior. If you did this as a package in your vimrc then you'd have to pick one or the other.

@timokau
Copy link
Member

timokau commented Mar 20, 2020

So basically the only difference is that vim will load the plugin and vimacs will not load the plugin?

@millerjason
Copy link
Contributor Author

So basically the only difference is that vim will load the plugin and vimacs will not load the plugin?

The wrapper will add the plugin to vim's RTP, enables insert mode, and enables the plugin. More importantly: under a non-GUI to also handles the tty changes necessary to avoid flow control problems (which would occur if you used the plugin outside of the script).

@timokau
Copy link
Member

timokau commented Mar 20, 2020

More importantly: under a non-GUI to also handles the tty changes necessary to avoid flow control problems (which would occur if you used the plugin outside of the script).

Can you explain that part?

One disadvantage of the wrapping approach is that you won't be able to use nixpkgs to further customize that vim instance (for example installing additional addons). Couldn't something equivalent be achieved with

vimacs = pkgs.vim_configurable.customize {
  name = "vimacs";

  vimrcConfig = {
    customRC = ''
      startinsert
    '';
    packages.vimacs = with pkgs.vimPlugins; {
      start = [ vimacs ];
    };
  };
};

?

@millerjason
Copy link
Contributor Author

More importantly: under a non-GUI to also handles the tty changes necessary to avoid flow control problems (which would occur if you used the plugin outside of the script).

Can you explain that part?

One disadvantage of the wrapping approach is that you won't be able to use nixpkgs to further customize that vim instance (for example installing additional addons). Couldn't something equivalent be achieved with

vimacs = pkgs.vim_configurable.customize {
  name = "vimacs";

  vimrcConfig = {
    customRC = ''
      startinsert
    '';
    packages.vimacs = with pkgs.vimPlugins; {
      start = [ vimacs ];
    };
  };
};

?

Emacs (and Vimacs, since it has the same key bindings) needs to pass through IXON, IXOFF when it is run, otherwise the terminal will capture C-s and C-q instead of pass it to vim. That has to be turned off as emacs does. One cannot do that in a vimrc file, as best as I know.

These are perhaps better questions for upstream. The wrapper script is upstream and intentional here.

@timokau
Copy link
Member

timokau commented Mar 20, 2020

I just tried it, and I can bind to C-s and C-q in plain vim. Have you tried the configuration I posted above?

@millerjason
Copy link
Contributor Author

I just tried it, and I can bind to C-s and C-q in plain vim. Have you tried the configuration I posted above?

Thanks for the suggestions, I'm definitely learning different ways to approach this in Nix. What you're suggesting is pretty neat, although I could not get it to work for me the same way.

First, I think we'd also want let g:VM_Enabled = 1, so I added that.

It does appear to work for C-s and C-q for me (maybe more recent versions of vim no longer need flow control overrides).

Still, it is not the same. Here are the differences I see immediately:

  • C-Z generates "^L:echo "Returning to Normal mode; press ^Z again to suspend Vimacs". Unfortunately, it also adds this to the working buffer. Maybe it's not functioning in command mode properly or not initializing the module properly? Normally this is a message, not added to the buffer. Some part is initialized incorrectly or in the wrong order.
  • I lose my vim status bar, although perhaps this is due to nix hiding my original dotfiles.
  • Other commands (vm, vmdiff, vimacsdiff) would also have to be added (the script handles all of these). That's not complicated, but unclear if this path reduces complexity.
  • vim_configurable has been broken on MacOS for years under Nix, so this fails to work for Mac

Thanks, Jason

Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

I cannot reproduce your point about C-z, but I do emphasize with the want to stay as close to upstream as possible. The biggest negative from this approach is the lack of customizability through nix, but then again maybe this is not intended for the kind of user who would want that (they could easily create their own customized vim_configurable).

All in all I have my reservations (questionable if upstream is still active, feels a bit hacky, quite some complexity for a probably not too popular package). But you seem highly motivated to get this in, and I think if there's one highly motivated user there are probably lots of others who haven't spoken up. So I tend towards including this. Maybe part of my reservations is also just my inherent "why would anybody want this" bias as a vim user ;)

@jonringer what do you think?

pkgs/applications/editors/vim/vimacs.nix Outdated Show resolved Hide resolved
@timokau
Copy link
Member

timokau commented Mar 23, 2020

@jonringer friendly ping, just in case this got lost in your email (normally I wouldn't ping so quickly, but since you reviewed #83119 I assume this got lost).

@jonringer
Copy link
Contributor

Maybe part of my reservations is also just my inherent "why would anybody want this" bias as a vim user ;)

I'm in a similar camp. But there may be a community around this, and I think @millerjason did a solid job trying to get it to fit within nixpkgs.

I think we should merge #83119 first as I think fixing conflicts will be easier on this branch

@jonringer friendly ping, just in case this got lost in your email (normally I wouldn't ping so quickly, but since you reviewed #83119 I assume this got lost).

No problem, I only have so many hours to address nixpkgs issues, what I ever I get to, I get to; many of them get dropped off :(

@millerjason
Copy link
Contributor Author

I'm in a similar camp. But there may be a community around this, and I think @millerjason did a solid job trying to get it to fit within nixpkgs.

I think we should merge #83119 first as I think fixing conflicts will be easier on this branch

@timokau @jonringer Thanks, much appreciated. Standing by to rebase once the other is merged.

@timokau
Copy link
Member

timokau commented Mar 27, 2020

#83119 is merged now, so after a rebase this should be good :)

@millerjason
Copy link
Contributor Author

#83119 is merged now, so after a rebase this should be good :)

Thanks -- rebase completed.

Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Thanks! I'll merge this once ofBorg has finished eval, as long as @jonringer doesn't beat me to it.

Welcome to the maintainers team :) Based on our interaction on this PR, I think you'll be a great addition and I'm looking forward to your future contributions if you decide to make any. I can even overlook your questionable taste in editors ;)

@timokau timokau merged commit c4f9734 into NixOS:master Mar 27, 2020
@millerjason
Copy link
Contributor Author

Thanks! I'll merge this once ofBorg has finished eval, as long as @jonringer doesn't beat me to it.

Welcome to the maintainers team :) Based on our interaction on this PR, I think you'll be a great addition and I'm looking forward to your future contributions if you decide to make any. I can even overlook your questionable taste in editors ;)

Thanks for reviewing the PR, guys - I appreciate the scope of your task to review so many of these and your thoughtfulness.

Now that Nix finally has a good editor to use, I'll be unencumbered to help more often. :)

@jonringer
Copy link
Contributor

Now that Nix finally has a good editor to use

The eternal war of editors, may it never stop

@millerjason millerjason deleted the add-vimacs branch March 27, 2020 19:35
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Mar 28, 2020
vimacs: init at 2016-03-24
(cherry picked from commit c4f9734)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants