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

Vim-like autocompletion in visual offset prompt #10912

Merged
merged 6 commits into from Aug 5, 2018

Conversation

@cyanpencil
Copy link
Contributor

@cyanpencil cyanpencil commented Aug 3, 2018

This is my attempt to address issue #8476

It is still a proof of concept, since for now it only works in visual offset prompt. But it works everywhere you can call the offset prompt (in visual mode, in panels mode, and in graph mode).

The idea is to make it as general as possible, so it can also be used elsewhere.

The major change here is not only in the visual representation of autocompletion, but also the fact that this offers autocompletion as-you-type.

Here's a gif of how it looks:
selection_widget3

The keybindings are more or less equal to vim's: up/down arrow keys or Ctrl-n/Ctrl-p to move selection, Enter to select item. Tab to invoke the autocompletion window

Feedbacks and code reviews are more than welcome! I tried to make the code as clean as possible and tried my best to avoid overly hackish things. But I believe it can still be improved.

I'll also move all those functions in a separate file (for now they're just all crammed in dietline.c)

@Maijin
Copy link
Contributor

@Maijin Maijin commented Aug 3, 2018

that looks amazing

@ret2libc
Copy link
Contributor

@ret2libc ret2libc commented Aug 3, 2018

OMG!! I'm going to review the code ASAP

Copy link
Contributor

@ret2libc ret2libc left a comment

I'm going to have another look soon and try out your branch

@@ -1557,6 +1727,10 @@ R_API const char *r_line_readline_cb(RLineReadCallback cb, void *user) {
fflush (stdout);
}

if (I.sel_widget) {

This comment has been minimized.

@ret2libc

ret2libc Aug 3, 2018
Contributor

no need to check for null before using free-like functions

@@ -956,6 +1098,10 @@ R_API const char *r_line_readline_cb_win(RLineReadCallback cb, void *user) {
fflush (stdout);
}

if (I.sel_widget) {

This comment has been minimized.

@ret2libc

ret2libc Aug 3, 2018
Contributor

no need to check for NULL in free-like functions

@@ -442,7 +442,112 @@ R_API int r_line_hist_chop(const char *file, int limit) {
return 0;
}

static void draw_selection_widget () {

This comment has been minimized.

@ret2libc

ret2libc Aug 3, 2018
Contributor

all these functions, according to the style, should have ( without a space before it.

char *background_color = cons->color ? "\x1b[40m" : Color_INVERT; // XXX colors from palette
char *selected_color = cons->color ? "\x1b[41m" : Color_INVERT_RESET; // XXX colors from palette
bool scrollbar = sel_widget->options_len > R_SELWIDGET_MAXH;
int scrollbar_y = scrollbar

This comment has been minimized.

@ret2libc

ret2libc Aug 3, 2018
Contributor

I think it would be clearer to just do:

int scrollbar_y = 0, scrollbar_l = 0;
if (scrollbar) {
   scrollbar_y = ...;
   scrollbar_l = ...;
}
r_cons_printf ("%-*.*s", sel_widget->w, sel_widget->w, option);
if (scrollbar) {
r_cons_printf ("%s", background_color);
r_cons_printf ("%s", R_BETWEEN(scrollbar_y, y, scrollbar_y + scrollbar_l) ? "" : " ");

This comment has been minimized.

@XVilka

XVilka Aug 3, 2018
Contributor

Please move this character in define.

This comment has been minimized.

@radare

radare Aug 4, 2018
Collaborator

Missing space before (

@XVilka XVilka requested a review from radare Aug 4, 2018
}
sel_widget->w = R_MIN (sel_widget->w, R_SELWIDGET_MAXW);

char *background_color = cons->color ? "\x1b[40m" : Color_INVERT; // XXX colors from palette

This comment has been minimized.

@radare

radare Aug 4, 2018
Collaborator

Dont hardcode ansi escapes. Use the Color_ macros

}

r_cons_gotoxy (pos_x + I.buffer.length, pos_y);
r_cons_printf ("%s", Color_RESET_BG);

This comment has been minimized.

@radare

radare Aug 4, 2018
Collaborator

Use rcons.memcat, no need to format a %s


r_cons_gotoxy (pos_x + I.buffer.length, pos_y);
r_cons_printf ("%s", Color_RESET_BG);
r_cons_flush();

This comment has been minimized.

@radare

radare Aug 4, 2018
Collaborator

Missing space

static void selection_widget_select () {
RSelWidget *sel_widget = I.sel_widget;
if (sel_widget && sel_widget->selection < sel_widget->options_len) {
strncpy (I.buffer.data, sel_widget->options[sel_widget->selection], R_LINE_BUFSIZE - 1);

This comment has been minimized.

@radare

radare Aug 4, 2018
Collaborator

Strncpy and strlen? Thats innefficient

r_cons_printf ("%-*.*s", sel_widget->w, sel_widget->w, option);
if (scrollbar) {
r_cons_printf ("%s", background_color);
r_cons_printf ("%s", R_BETWEEN(scrollbar_y, y, scrollbar_y + scrollbar_l) ? "" : " ");

This comment has been minimized.

@radare

radare Aug 4, 2018
Collaborator

Missing space before (

@XVilka
Copy link
Contributor

@XVilka XVilka commented Aug 4, 2018

@cyanpencil can you please update this one?

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Aug 4, 2018

Thanks for the reviews! I tried to address all your suggestions in the two previous commits.
Now colors are taken from the palette. If I find the time I'll update all colorthemes to support the new colors.
Also, now the autocompletion widget works in the open file prompt of visual panels (File > Open)

#define R_SELWIDGET_MAXH 15
#define R_SELWIDGET_MAXW 30

#define SELWIDGET_SCROLLCHAR ""

This comment has been minimized.

@radare

radare Aug 4, 2018
Collaborator

honor utf8 and use | or # or o, *, ... instead when disabled

r_cons_printf ("%-*.*s", sel_widget->w, sel_widget->w, option);
if (scrollbar) {
r_cons_strcat (background_color);
r_cons_printf ("%s", R_BETWEEN (scrollbar_y, y, scrollbar_y + scrollbar_l) ? SELWIDGET_SCROLLCHAR : " ");

This comment has been minimized.

@radare

radare Aug 4, 2018
Collaborator

SELWIDGET_SCROLLCHAR is utf8, you must set a var at the begining of the function to be that one or an ascii alternative when utf8 is not set

sel_widget->options_len = 0;
sel_widget->selection = -1;
draw_selection_widget ();
R_FREE(I.sel_widget);

This comment has been minimized.

@radare

radare Aug 4, 2018
Collaborator

missing space

@@ -487,6 +493,8 @@ typedef struct r_cons_t {

#define R_CONS_KEY_ESC 0x1b

#define ANSI_ESC_CLEAR_LINE "\r\x1b[0K\r"

This comment has been minimized.

@radare

radare Aug 4, 2018
Collaborator

its the only key prefixed with ANSI_. we may find a better name. the first \r is probably not needed. i think it was added to fix some bad behaviour in some terminal emulators.

@radare
Copy link
Collaborator

@radare radare commented Aug 4, 2018

The PR looks good. ill merge after you fix those last comments

@radare radare added this to the 2.8.0 milestone Aug 4, 2018
@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Aug 5, 2018

@radare

SELWIDGET_SCROLLCHAR is utf8, you must set a var at the begining of the function to be that one or an ascii alternative when utf8 is not set

Right, I should have checked it. However, after seeing that using #, ", *, etc instead of the block character gave horrible results, I opted to delete SELWIDGET_SCROLLCHAR and fake a block char by drawing a space with the inverted background color, like this:
r_cons_memcat (Color_INVERT" "Color_INVERT_RESET, 10)

its the only key prefixed with ANSI_. we may find a better name. the first \r is probably not needed. i think it was added to fix some bad behaviour in some terminal emulators.

Ok, renamed it to R_CONS_CLEAR_LINE. I removed the first \r, but had to switch from \x1b[0K to \x1b[2K. According to wikipedia, 0K clears from the cursor to the end of the line; 2K instead clears the whole line, independently of the cursor position. I had to change a small part in graph.c to adapt to this change. Tell me if you're okay with this.

@cyanpencil cyanpencil changed the title [WIP] Vim-like autocompletion in visual offset prompt Vim-like autocompletion in visual offset prompt Aug 5, 2018
@radare radare merged commit 370f35c into radareorg:master Aug 5, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants