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

Readline support #185

Closed
vivien opened this issue Aug 10, 2013 · 21 comments
Closed

Readline support #185

vivien opened this issue Aug 10, 2013 · 21 comments

Comments

@vivien
Copy link
Contributor

vivien commented Aug 10, 2013

It would be great to have the readline support for the prompt (commands and search).

I'm actually working on it and have a dirty working monkey patch.
@jonas I'd like you to try my readline branch and tell me if you'd agree for such a feature.

Actually, only the edition mode is supported, that means: moving the cursor, browsing the history (Up key) and reverse search (Ctrl-R). The vi mode is quite nice too. I'm already enjoying it!

My next step is to work on the completion support: filenames (builtin), tig commands and variable names.

Cheers!

@jonas
Copy link
Owner

jonas commented Aug 10, 2013

Awesome! :) I really like it.

@vivien
Copy link
Contributor Author

vivien commented Aug 10, 2013

Cool. I'll finish my work on the completion and let you know, I'd be able to have something cool this week end.
@jonas, towards splitting the source files, I thought about moving the prompt related code to prompt.[c,h] files (or readline). What do you think?

@jonas
Copy link
Owner

jonas commented Aug 10, 2013

Yes, that sounds great. And I see you also integrated it with the configure
script. Ideally, it should be optional somehow.

On Fri, Aug 9, 2013 at 11:54 PM, Vivien Didelot notifications@github.comwrote:

Cool. I'll finish my work on the completion and let you know, I'd be able
to have something cool this week end.
@jonas https://github.com/jonas, towards splitting the source files, I
thought about moving the prompt related code to prompt.[c,h] files (or
readline). What do you think?


Reply to this email directly or view it on GitHubhttps://github.com//issues/185#issuecomment-22433514
.

Jonas Fonseca

@jonas
Copy link
Owner

jonas commented Aug 20, 2013

This would partially fix #45

@vivien
Copy link
Contributor Author

vivien commented Aug 26, 2013

Update: I have something cool working here. We now have cursor navigation, history, and action completions.
You can try the readline branch, with for instance :vi<TAB>, :view-b<TAB>, ...

Other completions should come soon. The patch still needs some code and compatibility cleanup.

@jonas
Copy link
Owner

jonas commented Feb 24, 2014

I've started splitting the code. Among the stuff remaining is options (to complete :set) and argv formatting (to complete %(...)). Did I miss something?

@vivien
Copy link
Contributor Author

vivien commented Feb 24, 2014

I don't think so

jonas added a commit that referenced this issue Feb 28, 2014
This uses macros to define `struct argv_env` members as well as
argv_format variables and will simplify things for the readline
support.

References #185
@jonas
Copy link
Owner

jonas commented Feb 28, 2014

OK, both are now in place. Just include "options.h" and "argv.h" and use

const char *format_vars[] = {
#define ARGV_FORMAT_VAR(name, ifempty, initval) \
        { "%(" #name ")" }
        ARGV_ENV_INFO(ARGV_FORMAT_VAR)
};

const char *option_names[] = {
#define OPTION_NAME(name, type) #name,
        OPTION_INFO(OPTION_NAME)
};

@vivien
Copy link
Contributor Author

vivien commented Mar 1, 2014

Great! I'll look at that tomorrow.

@vivien
Copy link
Contributor Author

vivien commented Apr 3, 2014

@jonas, could you please give it another try and review please: https://github.com/vivien/tig/commit/77f4008d5079ebdf9a9f3bf8e5037f3e85bd4725.patch

@jonas
Copy link
Owner

jonas commented Apr 4, 2014

See my comments on the commit. I would also other to have readline
functions prefixed with readline_ or completion_ and then rename
tig_completion to prompt_completion or something like that.
On Apr 3, 2014 4:07 PM, "Vivien Didelot" notifications@github.com wrote:

@jonas https://github.com/jonas, could you please give it another try
and review please:
https://github.com/vivien/tig/commit/77f4008d5079ebdf9a9f3bf8e5037f3e85bd4725.patch


Reply to this email directly or view it on GitHubhttps://github.com//issues/185#issuecomment-39498276
.

@vivien
Copy link
Contributor Author

vivien commented Apr 7, 2014

Thanks for the review.
Here's the new version: https://github.com/vivien/tig/commits/readline
Please note that I've made two commits on your behalf:

  • argv: introduce argv_format_arg()
  • display: expose status_win and opt_tty

They were changes you've made to facilitate the readline support.
And then the readline patch itself: vivien@06103e4

It is not perfect yet, but the feature is there. Then we can add better/more completions and display menu.

@jonas jonas closed this as completed in 7dbf126 Apr 14, 2014
@jonas
Copy link
Owner

jonas commented Apr 14, 2014

Awesome, merged.

@yasuoza
Copy link
Contributor

yasuoza commented Apr 15, 2014

Readline support does not pass make with following error in OSX Marvericks.
I tried with commit 06103e4.

ozaki@macbook $ make                                    
        CC  src/tig.o
        CC  src/types.o
        CC  src/string.o
        CC  src/util.o
        CC  src/argv.o
        CC  src/io.o
        CC  src/graph.o
        CC  src/refdb.o
       GEN  src/builtin-config.c
        CC  src/builtin-config.o
        CC  src/request.o
        CC  src/line.o
        CC  src/keys.o
        CC  src/repo.o
        CC  src/options.o
        CC  src/draw.o
        CC  src/prompt.o
src/prompt.c:122:22: error: use of undeclared identifier 'rl_display_prompt'
        waddstr(status_win, rl_display_prompt);
                            ^
/usr/include/ncurses.h:999:39: note: expanded from macro 'waddstr'
#define waddstr(win,str)        waddnstr(win,str,-1)
                                             ^
src/prompt.c:125:30: error: use of undeclared identifier 'rl_display_prompt'
        wmove(status_win, 0, strlen(rl_display_prompt) + rl_point);
                                    ^
src/prompt.c:292:2: error: use of undeclared identifier 'rl_completion_suppress_append'
        rl_completion_suppress_append = 1;
        ^
src/prompt.c:361:37: warning: incompatible pointer types assigning to 'VFunction *' (aka 'void (*)(void)') from 'void (char **, int, int)' [-Wincompatible-pointer-types]
        rl_completion_display_matches_hook = readline_display_matches;
                                           ^ ~~~~~~~~~~~~~~~~~~~~~~~~
1 warning and 3 errors generated.
make: *** [src/prompt.o] Error 1

./configure outputs:

ozaki@macbook $ ./configure --prefix=`brew --prefix`           
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for gcc option to accept ISO C89... none needed
checking how to run the C preprocessor... gcc -E
checking for grep that handles long lines and -e... /usr/bin/grep
checking for egrep... /usr/bin/grep -E
checking for ANSI C header files... yes
checking for sys/types.h... yes
checking for sys/stat.h... yes
checking for stdlib.h... yes
checking for string.h... yes
checking for memory.h... yes
checking for strings.h... yes
checking for inttypes.h... yes
checking for stdint.h... yes
checking for unistd.h... yes
checking for stdint.h... (cached) yes
checking for stdlib.h... (cached) yes
checking for string.h... (cached) yes
checking sys/time.h usability... yes
checking sys/time.h presence... yes
checking for sys/time.h... yes
checking for unistd.h... (cached) yes
checking for gettimeofday... yes
checking whether environ is declared... no
checking whether errno is declared... yes
checking for mkstemps... yes
checking for setenv... yes
checking for NcursesW wide-character library... no
checking for Ncurses library... yes
checking for working ncurses/curses.h... no
checking for working ncurses.h... yes
configure: WARNING: The found ncurses library does not support wide-char.
configure: WARNING: This means that tig will not correctly render UTF-8.
checking for a readline compatible library... -lreadline
checking readline.h usability... no
checking readline.h presence... no
checking for readline.h... no
checking readline/readline.h usability... yes
checking readline/readline.h presence... yes
checking for readline/readline.h... yes
checking whether readline supports history... yes
checking history.h usability... no
checking history.h presence... no
checking for history.h... no
checking readline/history.h usability... yes
checking readline/history.h presence... yes
checking for readline/history.h... yes
checking for iconv... yes
checking for iconv declaration...
         extern size_t iconv (iconv_t cd, char * *inbuf, size_t *inbytesleft, char * *outbuf, size_t *outbytesleft);
checking for asciidoc... no
checking for xmlto... no
checking for docbook2pdf... no
configure: creating ./config.status
config.status: creating config.make
config.status: creating config.h

brew --prefix readline outputs:

ozaki@macbook $ brew --prefix readline
/usr/local/opt/readline

What's wrong?

@vivien
Copy link
Contributor Author

vivien commented Apr 15, 2014

Hi @yasuoza,

My bad, in this new version, I forgot to add the dependency tools/ax_lib_readline.m4, but @jonas added it back in the next commit f21329e. Sorry about that.

@jonas
Copy link
Owner

jonas commented Apr 15, 2014

The problem is that the readline library provided on Mac OS X by default is
not compatible with the version expected by this patch. So the
ax_lib_readline.m4 macros need to be extended to check for the right method
signatures.

On Tue, Apr 15, 2014 at 9:14 AM, Vivien Didelot notifications@github.comwrote:

Hi @yasuoza https://github.com/yasuoza,

That's my mistake. In this new version, I forgot to add the dependency
tools/ax_lib_readline.m4, but @jonas https://github.com/jonas added it
back in the next commit f21329ehttps://github.com/jonas/tig/commit/f21329e.
Sorry about that.


Reply to this email directly or view it on GitHubhttps://github.com//issues/185#issuecomment-40479390
.

Jonas Fonseca

@jonas
Copy link
Owner

jonas commented Apr 15, 2014

BTW, that's actually why the patch adds some additional checks in
contrib/config.make-Darwin

On Tue, Apr 15, 2014 at 10:49 AM, Jonas Fonseca jonas.fonseca@gmail.comwrote:

The problem is that the readline library provided on Mac OS X by default
is not compatible with the version expected by this patch. So the
ax_lib_readline.m4 macros need to be extended to check for the right method
signatures.

On Tue, Apr 15, 2014 at 9:14 AM, Vivien Didelot notifications@github.comwrote:

Hi @yasuoza https://github.com/yasuoza,

That's my mistake. In this new version, I forgot to add the dependency
tools/ax_lib_readline.m4, but @jonas https://github.com/jonas added it
back in the next commit f21329ehttps://github.com/jonas/tig/commit/f21329e.
Sorry about that.


Reply to this email directly or view it on GitHubhttps://github.com//issues/185#issuecomment-40479390
.

Jonas Fonseca

Jonas Fonseca

@vivien
Copy link
Contributor Author

vivien commented Apr 15, 2014

Thanks for fixing these compat issue @jonas.

@yasuoza
Copy link
Contributor

yasuoza commented Apr 16, 2014

Same error still happens in f893b0a(current master) 😵

jonas added a commit that referenced this issue Apr 16, 2014
For now the minimum version of readline is fixed at 6.2 which is what
brew currently provides. Certain previous versions probably also works.

This also introduces a --with-readline=DIR argument to configure, making
it simpler to tell configure where to look for readline.

Fixes #185
@jonas
Copy link
Owner

jonas commented Apr 16, 2014

@yasuoza Yes, it still happened because the autoconf macro only checked for existing of "some" readline library and not whether it is a version that is compatible with the prompt code.

Anyway, the latest version has been fixed.

@yasuoza
Copy link
Contributor

yasuoza commented Apr 16, 2014

Yeah! d780fac passed make! Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants