use editline instead of linenoise(-ng) #2228
Conversation
|
Observations so far:
|
|
Apparently editline is not already in upstream Nixpkgs so perhaps this is unwise? (apparently no other programs we package use it?) |
|
Unwise for what reason? |
|
General dependency "health", I suppose. If we are the only user it's ours to maintain/update. Sometimes there is a reason no one else uses a component, but I think we're safe here if it's what Debian uses. |
|
any progress here? The commit message still says WIP |
|
I don't remember what is "wip" about it, sorry (shame on me for not making a better note of this at at the time!). I feel we'd be better off using libedit but I think that'll take non-trivial effort since it uses a different API, but presumably nothing entirely too radically different :). |
|
FWIW I don't think working on this (either testing/improving what is started here, or exploring using libedit or something) requires much knowledge of anything Nix-internal, so it may be a good project for a wider audience of developers :). |
|
Not inserting a shared "prefix" among completions was making me sad, so put together a commit to provide that behavior. Pushing here since I think it makes editline more usable but I'm okay if would rather merge without. |
dtzWill
force-pushed
the
feature/editline-pr
branch
from
9b98e3d
to
af4f014
Compare
|
rebased (not quite to master due to #2249) and replaced the prefix completion with a slightly better approach. |
|
Might act a bit differently when editing long lines that wrap, especially as interacting with terminal resizing? |
[SNIP]
Hi @dtzWill, may I ask what is the reason for not using readline? From the perspective of packaging Nix for Debian it would be a huge step forward, because readline is already available as a package in Debian. libedit (http://thrysoee.dk/editline/) would also be available in Debian. editline (http://troglobit.com/projects/editline/) would be slightly more complicated, because it is not available in Debian (although there is a package by the name editline but it seems to be not related to the one you are using). |
|
We used to use readline, but got rid of it because it doesn't depend on ncurses (c5f23f1). |
|
I have created a commit on this branch that replaces editline by readline. Would it make sense to create a pull request from it or do you see other problems when using readline? [1]: It does not pull in ncurses as a dependency on Debian, but readline does indeed have ncurses in its closure on Nix (maybe as a termcap lib). |
src/nix/repl.cc
Outdated
| // and read brand new input. | ||
| linenoiseHistoryAdd(input.c_str()); | ||
| // We handled the current input fully, so we should clear it and read brand new input. | ||
| // TODO: history_add ? |
There was a problem hiding this comment.
I suppose this TODO still needs to be done to get proper history.
|
On Mon, 29 Oct 2018 05:09:08 -0700, Eelco Dolstra ***@***.***> wrote:
edolstra commented on this pull request.
> @@ -173,9 +233,8 @@ void NixRepl::mainLoop(const std::vector<std::string> & files)
printMsg(lvlError, format(error + "%1%%2%") % (settings.showTrace ? e.prefix() : "") % e.msg());
}
- // We handled the current input fully, so we should clear it
- // and read brand new input.
- linenoiseHistoryAdd(input.c_str());
+ // We handled the current input fully, so we should clear it and read brand new input.
+ // TODO: history_add ?
I suppose this TODO still needs to be done to get proper history.
I believe editline will add to history automatically, or at least a
comment in the editline source suggests as much :).
Building to check presently..
Yep, history works here. Can probably drop the TODO?
(unless the auto-history is undesired/wrong, but at least basic usage
works as I expect)
…
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#2228 (review) part: text/html
|
|
Conflicts resolved (merge, not rebase eep): https://github.com/dtzWill/nix/tree/feature/editline-pr-merge Which also contains #2485 so it builds against 18.09 for testing. Will tackle a rebase and drop the TODO in a few... |
dtzWill
force-pushed
the
feature/editline-pr
branch
from
af4f014
to
9f99809
Compare
Bump fallback editline expression to latest in nixpkgs.
|
Thanks, merged! |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
I marked the commit as "WIP" but I don't remember
what problems or concerns I had with this.
Poking at it, things seem to work nicely?
Not sure if utf8 is supported, don't think this is needed
for anything we have in nixpkgs.
Maybe I was just unhappy about adding another dependency?
Fixes #2014 .
May be good to review recent completion bugs and look for regressions.