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
Conversation
that looks amazing |
OMG!! I'm going to review the code ASAP |
There was a problem hiding this 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
libr/cons/dietline.c
Outdated
@@ -1557,6 +1727,10 @@ R_API const char *r_line_readline_cb(RLineReadCallback cb, void *user) { | |||
fflush (stdout); | |||
} | |||
|
|||
if (I.sel_widget) { |
There was a problem hiding this comment.
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
libr/cons/dietline.c
Outdated
@@ -956,6 +1098,10 @@ R_API const char *r_line_readline_cb_win(RLineReadCallback cb, void *user) { | |||
fflush (stdout); | |||
} | |||
|
|||
if (I.sel_widget) { |
There was a problem hiding this comment.
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
libr/cons/dietline.c
Outdated
@@ -442,7 +442,112 @@ R_API int r_line_hist_chop(const char *file, int limit) { | |||
return 0; | |||
} | |||
|
|||
static void draw_selection_widget () { |
There was a problem hiding this comment.
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.
libr/cons/dietline.c
Outdated
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 |
There was a problem hiding this comment.
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 = ...;
}
libr/cons/dietline.c
Outdated
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) ? "█" : " "); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space before (
libr/cons/dietline.c
Outdated
} | ||
sel_widget->w = R_MIN (sel_widget->w, R_SELWIDGET_MAXW); | ||
|
||
char *background_color = cons->color ? "\x1b[40m" : Color_INVERT; // XXX colors from palette |
There was a problem hiding this comment.
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
libr/cons/dietline.c
Outdated
} | ||
|
||
r_cons_gotoxy (pos_x + I.buffer.length, pos_y); | ||
r_cons_printf ("%s", Color_RESET_BG); |
There was a problem hiding this comment.
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
libr/cons/dietline.c
Outdated
|
||
r_cons_gotoxy (pos_x + I.buffer.length, pos_y); | ||
r_cons_printf ("%s", Color_RESET_BG); | ||
r_cons_flush(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space
libr/cons/dietline.c
Outdated
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); |
There was a problem hiding this comment.
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
libr/cons/dietline.c
Outdated
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) ? "█" : " "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space before (
@cyanpencil can you please update this one? |
Thanks for the reviews! I tried to address all your suggestions in the two previous commits. |
libr/include/r_cons.h
Outdated
#define R_SELWIDGET_MAXH 15 | ||
#define R_SELWIDGET_MAXW 30 | ||
|
||
#define SELWIDGET_SCROLLCHAR "█" |
There was a problem hiding this comment.
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
libr/cons/dietline.c
Outdated
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 : " "); |
There was a problem hiding this comment.
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
libr/cons/dietline.c
Outdated
sel_widget->options_len = 0; | ||
sel_widget->selection = -1; | ||
draw_selection_widget (); | ||
R_FREE(I.sel_widget); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing space
libr/include/r_cons.h
Outdated
@@ -487,6 +493,8 @@ typedef struct r_cons_t { | |||
|
|||
#define R_CONS_KEY_ESC 0x1b | |||
|
|||
#define ANSI_ESC_CLEAR_LINE "\r\x1b[0K\r" |
There was a problem hiding this comment.
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.
The PR looks good. ill merge after you fix those last comments |
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:
Ok, renamed it to R_CONS_CLEAR_LINE. I removed the first \r, but had to switch from |
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:
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)