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 commented Aug 3, 2018

that looks amazing

@ret2libc
Copy link
Contributor

ret2libc commented Aug 3, 2018

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

Copy link
Contributor

@ret2libc ret2libc left a comment

Choose a reason for hiding this comment

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 () {
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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) ? "█" : " ");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this character in define.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing space before (

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

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

Choose a reason for hiding this comment

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

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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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) ? "█" : " ");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing space before (

@XVilka
Copy link
Contributor

XVilka commented Aug 4, 2018

@cyanpencil can you please update this one?

@cyanpencil
Copy link
Contributor Author

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 "█"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 : " ");
Copy link
Collaborator

Choose a reason for hiding this comment

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

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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 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

@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
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

5 participants