2023-06-29 16:26:15

by Masahiro Yamada

[permalink] [raw]
Subject: [PATCH 1/2] kconfig: menuconfig: simplify global jump key assignment

Commit 95ac9b3b585d ("menuconfig: Assign jump keys per-page instead
of globally") injects a lot of hacks to the bottom of the textbox
infrastructure.

I reverted many of them without changing the behavior. (almost)
Now, the key markers are inserted when constructing the search result
instead of updating the text buffer on-the-fly.

The buffer passed to the textbox got back to a constant string.
The ugly casts from (const char *) to (char *) went away.

A disadvantage is that the same key numbers might be diplayed multiple
times in the dialog if you use a huge window (but I believe it is
unlikely to happen).

Signed-off-by: Masahiro Yamada <[email protected]>
---

scripts/kconfig/lkc.h | 1 +
scripts/kconfig/lxdialog/dialog.h | 10 ++--
scripts/kconfig/lxdialog/textbox.c | 68 +++++++++--------------
scripts/kconfig/mconf.c | 86 +++++++++++++++++-------------
scripts/kconfig/menu.c | 22 ++++++--
5 files changed, 97 insertions(+), 90 deletions(-)

diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
index e7118d62a45f..d5c27180ce91 100644
--- a/scripts/kconfig/lkc.h
+++ b/scripts/kconfig/lkc.h
@@ -101,6 +101,7 @@ const char *menu_get_prompt(struct menu *menu);
struct menu *menu_get_parent_menu(struct menu *menu);
bool menu_has_help(struct menu *menu);
const char *menu_get_help(struct menu *menu);
+int get_jump_key(void);
struct gstr get_relations_str(struct symbol **sym_arr, struct list_head *head);
void menu_get_ext_help(struct menu *menu, struct gstr *help);

diff --git a/scripts/kconfig/lxdialog/dialog.h b/scripts/kconfig/lxdialog/dialog.h
index 347daf25fdc8..cd1b59c24b21 100644
--- a/scripts/kconfig/lxdialog/dialog.h
+++ b/scripts/kconfig/lxdialog/dialog.h
@@ -196,13 +196,9 @@ int first_alpha(const char *string, const char *exempt);
int dialog_yesno(const char *title, const char *prompt, int height, int width);
int dialog_msgbox(const char *title, const char *prompt, int height,
int width, int pause);
-
-
-typedef void (*update_text_fn)(char *buf, size_t start, size_t end, void
- *_data);
-int dialog_textbox(const char *title, char *tbuf, int initial_height,
- int initial_width, int *keys, int *_vscroll, int *_hscroll,
- update_text_fn update_text, void *data);
+int dialog_textbox(const char *title, const char *tbuf, int initial_height,
+ int initial_width, int *_vscroll, int *_hscroll,
+ int (*extra_key_cb)(int, int, int, void *), void *data);
int dialog_menu(const char *title, const char *prompt,
const void *selected, int *s_scroll);
int dialog_checklist(const char *title, const char *prompt, int height,
diff --git a/scripts/kconfig/lxdialog/textbox.c b/scripts/kconfig/lxdialog/textbox.c
index bc4d4fb1dc75..e6cd7bb83746 100644
--- a/scripts/kconfig/lxdialog/textbox.c
+++ b/scripts/kconfig/lxdialog/textbox.c
@@ -10,8 +10,8 @@

static int hscroll;
static int begin_reached, end_reached, page_length;
-static char *buf;
-static char *page;
+static const char *buf, *page;
+static int start, end;

/*
* Go back 'n' lines in text. Called by dialog_textbox().
@@ -98,21 +98,10 @@ static void print_line(WINDOW *win, int row, int width)
/*
* Print a new page of text.
*/
-static void print_page(WINDOW *win, int height, int width, update_text_fn
- update_text, void *data)
+static void print_page(WINDOW *win, int height, int width)
{
int i, passed_end = 0;

- if (update_text) {
- char *end;
-
- for (i = 0; i < height; i++)
- get_line();
- end = page;
- back_lines(height);
- update_text(buf, page - buf, end - buf, data);
- }
-
page_length = 0;
for (i = 0; i < height; i++) {
print_line(win, i, width);
@@ -142,24 +131,26 @@ static void print_position(WINDOW *win)
* refresh window content
*/
static void refresh_text_box(WINDOW *dialog, WINDOW *box, int boxh, int boxw,
- int cur_y, int cur_x, update_text_fn update_text,
- void *data)
+ int cur_y, int cur_x)
{
- print_page(box, boxh, boxw, update_text, data);
+ start = page - buf;
+
+ print_page(box, boxh, boxw);
print_position(dialog);
wmove(dialog, cur_y, cur_x); /* Restore cursor position */
wrefresh(dialog);
+
+ end = page - buf;
}

/*
* Display text from a file in a dialog box.
*
* keys is a null-terminated array
- * update_text() may not add or remove any '\n' or '\0' in tbuf
*/
-int dialog_textbox(const char *title, char *tbuf, int initial_height,
- int initial_width, int *keys, int *_vscroll, int *_hscroll,
- update_text_fn update_text, void *data)
+int dialog_textbox(const char *title, const char *tbuf, int initial_height,
+ int initial_width, int *_vscroll, int *_hscroll,
+ int (*extra_key_cb)(int, int, int, void *), void *data)
{
int i, x, y, cur_x, cur_y, key = 0;
int height, width, boxh, boxw;
@@ -239,8 +230,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,

/* Print first page of text */
attr_clear(box, boxh, boxw, dlg.dialog.atr);
- refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x, update_text,
- data);
+ refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);

while (!done) {
key = wgetch(dialog);
@@ -259,8 +249,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
begin_reached = 1;
page = buf;
refresh_text_box(dialog, box, boxh, boxw,
- cur_y, cur_x, update_text,
- data);
+ cur_y, cur_x);
}
break;
case 'G': /* Last page */
@@ -270,8 +259,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
/* point to last char in buf */
page = buf + strlen(buf);
back_lines(boxh);
- refresh_text_box(dialog, box, boxh, boxw, cur_y,
- cur_x, update_text, data);
+ refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
break;
case 'K': /* Previous line */
case 'k':
@@ -280,8 +268,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
break;

back_lines(page_length + 1);
- refresh_text_box(dialog, box, boxh, boxw, cur_y,
- cur_x, update_text, data);
+ refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
break;
case 'B': /* Previous page */
case 'b':
@@ -290,8 +277,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
if (begin_reached)
break;
back_lines(page_length + boxh);
- refresh_text_box(dialog, box, boxh, boxw, cur_y,
- cur_x, update_text, data);
+ refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
break;
case 'J': /* Next line */
case 'j':
@@ -300,8 +286,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
break;

back_lines(page_length - 1);
- refresh_text_box(dialog, box, boxh, boxw, cur_y,
- cur_x, update_text, data);
+ refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
break;
case KEY_NPAGE: /* Next page */
case ' ':
@@ -310,8 +295,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
break;

begin_reached = 0;
- refresh_text_box(dialog, box, boxh, boxw, cur_y,
- cur_x, update_text, data);
+ refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
break;
case '0': /* Beginning of line */
case 'H': /* Scroll left */
@@ -326,8 +310,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
hscroll--;
/* Reprint current page to scroll horizontally */
back_lines(page_length);
- refresh_text_box(dialog, box, boxh, boxw, cur_y,
- cur_x, update_text, data);
+ refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
break;
case 'L': /* Scroll right */
case 'l':
@@ -337,8 +320,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
hscroll++;
/* Reprint current page to scroll horizontally */
back_lines(page_length);
- refresh_text_box(dialog, box, boxh, boxw, cur_y,
- cur_x, update_text, data);
+ refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
break;
case KEY_ESC:
if (on_key_esc(dialog) == KEY_ESC)
@@ -351,11 +333,9 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
on_key_resize();
goto do_resize;
default:
- for (i = 0; keys[i]; i++) {
- if (key == keys[i]) {
- done = true;
- break;
- }
+ if (extra_key_cb(key, start, end, data)) {
+ done = true;
+ break;
}
}
}
diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
index 53d8834d12fe..7adfd6537279 100644
--- a/scripts/kconfig/mconf.c
+++ b/scripts/kconfig/mconf.c
@@ -288,6 +288,7 @@ static int single_menu_mode;
static int show_all_options;
static int save_and_exit;
static int silent;
+static int jump_key;

static void conf(struct menu *menu, struct menu *active_menu);

@@ -348,19 +349,19 @@ static void reset_subtitle(void)
set_dialog_subtitles(subtitles);
}

-static int show_textbox_ext(const char *title, char *text, int r, int c, int
- *keys, int *vscroll, int *hscroll, update_text_fn
- update_text, void *data)
+static int show_textbox_ext(const char *title, const char *text, int r, int c,
+ int *vscroll, int *hscroll,
+ int (*extra_key_cb)(int, int, int, void *),
+ void *data)
{
dialog_clear();
- return dialog_textbox(title, text, r, c, keys, vscroll, hscroll,
- update_text, data);
+ return dialog_textbox(title, text, r, c, vscroll, hscroll,
+ extra_key_cb, data);
}

static void show_textbox(const char *title, const char *text, int r, int c)
{
- show_textbox_ext(title, (char *) text, r, c, (int []) {0}, NULL, NULL,
- NULL, NULL);
+ show_textbox_ext(title, text, r, c, NULL, NULL, NULL, NULL);
}

static void show_helptext(const char *title, const char *text)
@@ -381,35 +382,51 @@ static void show_help(struct menu *menu)

struct search_data {
struct list_head *head;
- struct menu **targets;
- int *keys;
+ struct menu *target;
};

-static void update_text(char *buf, size_t start, size_t end, void *_data)
+static int next_key(int key)
+{
+ key++;
+
+ if (key > '9')
+ key = '1';
+
+ return key;
+}
+
+static int handle_search_keys(int key, int start, int end, void *_data)
{
struct search_data *data = _data;
struct jump_key *pos;
- int k = 0;
+
+ if (key < '1' || key > '9')
+ return 0;

list_for_each_entry(pos, data->head, entries) {
- if (pos->offset >= start && pos->offset < end) {
- char header[4];
+ if (pos->offset >= start) {
+ if (pos->offset >= end)
+ break;

- if (k < JUMP_NB) {
- int key = '0' + (pos->index % JUMP_NB) + 1;
-
- sprintf(header, "(%c)", key);
- data->keys[k] = key;
- data->targets[k] = pos->target;
- k++;
- } else {
- sprintf(header, " ");
+ if (key == '1' + (pos->index % JUMP_NB)) {
+ data->target = pos->target;
+ return 1;
}
-
- memcpy(buf + pos->offset, header, sizeof(header) - 1);
}
}
- data->keys[k] = 0;
+
+ return 0;
+}
+
+int get_jump_key(void)
+{
+ int cur_key;
+
+ cur_key = jump_key;
+
+ jump_key = next_key(cur_key);
+
+ return cur_key;
}

static void search_conf(void)
@@ -456,26 +473,23 @@ static void search_conf(void)
sym_arr = sym_re_search(dialog_input);
do {
LIST_HEAD(head);
- struct menu *targets[JUMP_NB];
- int keys[JUMP_NB + 1], i;
struct search_data data = {
.head = &head,
- .targets = targets,
- .keys = keys,
};
struct jump_key *pos, *tmp;

+ jump_key = '1';
res = get_relations_str(sym_arr, &head);
set_subtitle();
dres = show_textbox_ext("Search Results", str_get(&res), 0, 0,
- keys, &vscroll, &hscroll, &update_text,
- &data);
+ &vscroll, &hscroll,
+ handle_search_keys, &data);
again = false;
- for (i = 0; i < JUMP_NB && keys[i]; i++)
- if (dres == keys[i]) {
- conf(targets[i]->parent, targets[i]);
- again = true;
- }
+ if (dres >= '1' && dres <= '9') {
+ assert(data.target != NULL);
+ conf(data.target->parent, data.target);
+ again = true;
+ }
str_free(&res);
list_for_each_entry_safe(pos, tmp, &head, entries)
free(pos);
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index b90fff833588..5578b8bc8a23 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -701,6 +701,11 @@ static void get_dep_str(struct gstr *r, struct expr *expr, const char *prefix)
}
}

+int __attribute__((weak)) get_jump_key(void)
+{
+ return -1;
+}
+
static void get_prompt_str(struct gstr *r, struct property *prop,
struct list_head *head)
{
@@ -743,11 +748,22 @@ static void get_prompt_str(struct gstr *r, struct property *prop,
}

str_printf(r, " Location:\n");
- for (j = 4; --i >= 0; j += 2) {
+ for (j = 0; --i >= 0; j++) {
+ int jk = -1;
+ int indent = 2 * j + 4;
+
menu = submenu[i];
- if (jump && menu == location)
+ if (jump && menu == location) {
jump->offset = strlen(r->s);
- str_printf(r, "%*c-> %s", j, ' ', menu_get_prompt(menu));
+ jk = get_jump_key();
+ }
+
+ if (jk >= 0) {
+ str_printf(r, "(%c)", jk);
+ indent -= 3;
+ }
+
+ str_printf(r, "%*c-> %s", indent, ' ', menu_get_prompt(menu));
if (menu->sym) {
str_printf(r, " (%s [=%s])", menu->sym->name ?
menu->sym->name : "<choice>",
--
2.39.2



2023-07-01 04:27:25

by Jesse T

[permalink] [raw]
Subject: Re: [PATCH 1/2] kconfig: menuconfig: simplify global jump key assignment

On Thu, Jun 29, 2023 at 12:03 PM Masahiro Yamada <[email protected]> wrote:
>
> Commit 95ac9b3b585d ("menuconfig: Assign jump keys per-page instead
> of globally") injects a lot of hacks to the bottom of the textbox
> infrastructure.
>
> I reverted many of them without changing the behavior. (almost)
> Now, the key markers are inserted when constructing the search result
> instead of updating the text buffer on-the-fly.
>
> The buffer passed to the textbox got back to a constant string.
> The ugly casts from (const char *) to (char *) went away.
>
> A disadvantage is that the same key numbers might be diplayed multiple
> times in the dialog if you use a huge window (but I believe it is
> unlikely to happen).
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
> scripts/kconfig/lkc.h | 1 +
> scripts/kconfig/lxdialog/dialog.h | 10 ++--
> scripts/kconfig/lxdialog/textbox.c | 68 +++++++++--------------
> scripts/kconfig/mconf.c | 86 +++++++++++++++++-------------
> scripts/kconfig/menu.c | 22 ++++++--
> 5 files changed, 97 insertions(+), 90 deletions(-)
>
> diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
> index e7118d62a45f..d5c27180ce91 100644
> --- a/scripts/kconfig/lkc.h
> +++ b/scripts/kconfig/lkc.h
> @@ -101,6 +101,7 @@ const char *menu_get_prompt(struct menu *menu);
> struct menu *menu_get_parent_menu(struct menu *menu);
> bool menu_has_help(struct menu *menu);
> const char *menu_get_help(struct menu *menu);
> +int get_jump_key(void);
> struct gstr get_relations_str(struct symbol **sym_arr, struct list_head *head);
> void menu_get_ext_help(struct menu *menu, struct gstr *help);
>
> diff --git a/scripts/kconfig/lxdialog/dialog.h b/scripts/kconfig/lxdialog/dialog.h
> index 347daf25fdc8..cd1b59c24b21 100644
> --- a/scripts/kconfig/lxdialog/dialog.h
> +++ b/scripts/kconfig/lxdialog/dialog.h
> @@ -196,13 +196,9 @@ int first_alpha(const char *string, const char *exempt);
> int dialog_yesno(const char *title, const char *prompt, int height, int width);
> int dialog_msgbox(const char *title, const char *prompt, int height,
> int width, int pause);
> -
> -
> -typedef void (*update_text_fn)(char *buf, size_t start, size_t end, void
> - *_data);
> -int dialog_textbox(const char *title, char *tbuf, int initial_height,
> - int initial_width, int *keys, int *_vscroll, int *_hscroll,
> - update_text_fn update_text, void *data);
> +int dialog_textbox(const char *title, const char *tbuf, int initial_height,
> + int initial_width, int *_vscroll, int *_hscroll,
> + int (*extra_key_cb)(int, int, int, void *), void *data);
> int dialog_menu(const char *title, const char *prompt,
> const void *selected, int *s_scroll);
> int dialog_checklist(const char *title, const char *prompt, int height,
> diff --git a/scripts/kconfig/lxdialog/textbox.c b/scripts/kconfig/lxdialog/textbox.c
> index bc4d4fb1dc75..e6cd7bb83746 100644
> --- a/scripts/kconfig/lxdialog/textbox.c
> +++ b/scripts/kconfig/lxdialog/textbox.c
> @@ -10,8 +10,8 @@
>
> static int hscroll;
> static int begin_reached, end_reached, page_length;
> -static char *buf;
> -static char *page;
> +static const char *buf, *page;
> +static int start, end;
>
> /*
> * Go back 'n' lines in text. Called by dialog_textbox().
> @@ -98,21 +98,10 @@ static void print_line(WINDOW *win, int row, int width)
> /*
> * Print a new page of text.
> */
> -static void print_page(WINDOW *win, int height, int width, update_text_fn
> - update_text, void *data)
> +static void print_page(WINDOW *win, int height, int width)
> {
> int i, passed_end = 0;
>
> - if (update_text) {
> - char *end;
> -
> - for (i = 0; i < height; i++)
> - get_line();
> - end = page;
> - back_lines(height);
> - update_text(buf, page - buf, end - buf, data);
> - }
> -
> page_length = 0;
> for (i = 0; i < height; i++) {
> print_line(win, i, width);
> @@ -142,24 +131,26 @@ static void print_position(WINDOW *win)
> * refresh window content
> */
> static void refresh_text_box(WINDOW *dialog, WINDOW *box, int boxh, int boxw,
> - int cur_y, int cur_x, update_text_fn update_text,
> - void *data)
> + int cur_y, int cur_x)

The change for refresh_text_box is very large.
Is there an easy way to split the change of `refresh_text_box` and
everything else
while still maintaining bisectability?

> {
> - print_page(box, boxh, boxw, update_text, data);
> + start = page - buf;
> +
> + print_page(box, boxh, boxw);
> print_position(dialog);
> wmove(dialog, cur_y, cur_x); /* Restore cursor position */
> wrefresh(dialog);
> +
> + end = page - buf;
> }
>
> /*
> * Display text from a file in a dialog box.
> *
> * keys is a null-terminated array
> - * update_text() may not add or remove any '\n' or '\0' in tbuf
> */
> -int dialog_textbox(const char *title, char *tbuf, int initial_height,
> - int initial_width, int *keys, int *_vscroll, int *_hscroll,
> - update_text_fn update_text, void *data)
> +int dialog_textbox(const char *title, const char *tbuf, int initial_height,
> + int initial_width, int *_vscroll, int *_hscroll,
> + int (*extra_key_cb)(int, int, int, void *), void *data)
> {
> int i, x, y, cur_x, cur_y, key = 0;
> int height, width, boxh, boxw;
> @@ -239,8 +230,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
>
> /* Print first page of text */
> attr_clear(box, boxh, boxw, dlg.dialog.atr);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x, update_text,
> - data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
>
> while (!done) {
> key = wgetch(dialog);
> @@ -259,8 +249,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> begin_reached = 1;
> page = buf;
> refresh_text_box(dialog, box, boxh, boxw,
> - cur_y, cur_x, update_text,
> - data);
> + cur_y, cur_x);
> }
> break;
> case 'G': /* Last page */
> @@ -270,8 +259,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> /* point to last char in buf */
> page = buf + strlen(buf);
> back_lines(boxh);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case 'K': /* Previous line */
> case 'k':
> @@ -280,8 +268,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> break;
>
> back_lines(page_length + 1);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case 'B': /* Previous page */
> case 'b':
> @@ -290,8 +277,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> if (begin_reached)
> break;
> back_lines(page_length + boxh);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case 'J': /* Next line */
> case 'j':
> @@ -300,8 +286,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> break;
>
> back_lines(page_length - 1);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case KEY_NPAGE: /* Next page */
> case ' ':
> @@ -310,8 +295,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> break;
>
> begin_reached = 0;
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case '0': /* Beginning of line */
> case 'H': /* Scroll left */
> @@ -326,8 +310,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> hscroll--;
> /* Reprint current page to scroll horizontally */
> back_lines(page_length);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case 'L': /* Scroll right */
> case 'l':
> @@ -337,8 +320,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> hscroll++;
> /* Reprint current page to scroll horizontally */
> back_lines(page_length);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case KEY_ESC:
> if (on_key_esc(dialog) == KEY_ESC)
> @@ -351,11 +333,9 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> on_key_resize();
> goto do_resize;
> default:
> - for (i = 0; keys[i]; i++) {
> - if (key == keys[i]) {
> - done = true;
> - break;
> - }
> + if (extra_key_cb(key, start, end, data)) {

`extra_key_cb` is null when not used, on the help page this will segfault.

if (extra_key_cb && extra_key_cb(key, start, end, data)) {

> + done = true;
> + break;
> }
> }
> }
> diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
> index 53d8834d12fe..7adfd6537279 100644
> --- a/scripts/kconfig/mconf.c
> +++ b/scripts/kconfig/mconf.c
> @@ -288,6 +288,7 @@ static int single_menu_mode;
> static int show_all_options;
> static int save_and_exit;
> static int silent;
> +static int jump_key;
>
> static void conf(struct menu *menu, struct menu *active_menu);
>
> @@ -348,19 +349,19 @@ static void reset_subtitle(void)
> set_dialog_subtitles(subtitles);
> }
>
> -static int show_textbox_ext(const char *title, char *text, int r, int c, int
> - *keys, int *vscroll, int *hscroll, update_text_fn
> - update_text, void *data)
> +static int show_textbox_ext(const char *title, const char *text, int r, int c,
> + int *vscroll, int *hscroll,
> + int (*extra_key_cb)(int, int, int, void *),
> + void *data)
> {
> dialog_clear();
> - return dialog_textbox(title, text, r, c, keys, vscroll, hscroll,
> - update_text, data);
> + return dialog_textbox(title, text, r, c, vscroll, hscroll,
> + extra_key_cb, data);
> }
>
> static void show_textbox(const char *title, const char *text, int r, int c)
> {
> - show_textbox_ext(title, (char *) text, r, c, (int []) {0}, NULL, NULL,
> - NULL, NULL);
> + show_textbox_ext(title, text, r, c, NULL, NULL, NULL, NULL);
> }
>
> static void show_helptext(const char *title, const char *text)
> @@ -381,35 +382,51 @@ static void show_help(struct menu *menu)
>
> struct search_data {
> struct list_head *head;
> - struct menu **targets;
> - int *keys;
> + struct menu *target;
> };
>
> -static void update_text(char *buf, size_t start, size_t end, void *_data)
> +static int next_key(int key)
> +{
> + key++;
> +
> + if (key > '9')
> + key = '1';
> +
> + return key;
> +}
> +
> +static int handle_search_keys(int key, int start, int end, void *_data)
> {
> struct search_data *data = _data;
> struct jump_key *pos;
> - int k = 0;
> +
> + if (key < '1' || key > '9')
> + return 0;
>
> list_for_each_entry(pos, data->head, entries) {
> - if (pos->offset >= start && pos->offset < end) {
> - char header[4];
> + if (pos->offset >= start) {
> + if (pos->offset >= end)
> + break;
>
> - if (k < JUMP_NB) {
> - int key = '0' + (pos->index % JUMP_NB) + 1;
> -
> - sprintf(header, "(%c)", key);
> - data->keys[k] = key;
> - data->targets[k] = pos->target;
> - k++;
> - } else {
> - sprintf(header, " ");
> + if (key == '1' + (pos->index % JUMP_NB)) {
> + data->target = pos->target;
> + return 1;
> }
> -
> - memcpy(buf + pos->offset, header, sizeof(header) - 1);
> }
> }
> - data->keys[k] = 0;
> +
> + return 0;
> +}
> +
> +int get_jump_key(void)
> +{
> + int cur_key;
> +
> + cur_key = jump_key;
> +
> + jump_key = next_key(cur_key);
> +
> + return cur_key;
> }
>
> static void search_conf(void)
> @@ -456,26 +473,23 @@ static void search_conf(void)
> sym_arr = sym_re_search(dialog_input);
> do {
> LIST_HEAD(head);
> - struct menu *targets[JUMP_NB];
> - int keys[JUMP_NB + 1], i;
> struct search_data data = {
> .head = &head,
> - .targets = targets,
> - .keys = keys,
> };
> struct jump_key *pos, *tmp;
>
> + jump_key = '1';
> res = get_relations_str(sym_arr, &head);
> set_subtitle();
> dres = show_textbox_ext("Search Results", str_get(&res), 0, 0,
> - keys, &vscroll, &hscroll, &update_text,
> - &data);
> + &vscroll, &hscroll,
> + handle_search_keys, &data);
> again = false;
> - for (i = 0; i < JUMP_NB && keys[i]; i++)
> - if (dres == keys[i]) {
> - conf(targets[i]->parent, targets[i]);
> - again = true;
> - }
> + if (dres >= '1' && dres <= '9') {
> + assert(data.target != NULL);
> + conf(data.target->parent, data.target);
> + again = true;
> + }
> str_free(&res);
> list_for_each_entry_safe(pos, tmp, &head, entries)
> free(pos);
> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index b90fff833588..5578b8bc8a23 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -701,6 +701,11 @@ static void get_dep_str(struct gstr *r, struct expr *expr, const char *prefix)
> }
> }
>
> +int __attribute__((weak)) get_jump_key(void)

This seems like a non-optimal solution, otherwise fine.

Other than the call to null, looks good. After that change.

Reviewed-by: Jesse Taube <[email protected]>

Thanks,
Jesse Taube

> +{
> + return -1;
> +}
> +
> static void get_prompt_str(struct gstr *r, struct property *prop,
> struct list_head *head)
> {
> @@ -743,11 +748,22 @@ static void get_prompt_str(struct gstr *r, struct property *prop,
> }
>
> str_printf(r, " Location:\n");
> - for (j = 4; --i >= 0; j += 2) {
> + for (j = 0; --i >= 0; j++) {
> + int jk = -1;
> + int indent = 2 * j + 4;
> +
> menu = submenu[i];
> - if (jump && menu == location)
> + if (jump && menu == location) {
> jump->offset = strlen(r->s);
> - str_printf(r, "%*c-> %s", j, ' ', menu_get_prompt(menu));
> + jk = get_jump_key();
> + }
> +
> + if (jk >= 0) {
> + str_printf(r, "(%c)", jk);
> + indent -= 3;
> + }
> +
> + str_printf(r, "%*c-> %s", indent, ' ', menu_get_prompt(menu));
> if (menu->sym) {
> str_printf(r, " (%s [=%s])", menu->sym->name ?
> menu->sym->name : "<choice>",
> --
> 2.39.2
>

2023-07-02 15:36:50

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH 1/2] kconfig: menuconfig: simplify global jump key assignment

On Sat, Jul 1, 2023 at 12:58 PM Jesse T <[email protected]> wrote:
>
> On Thu, Jun 29, 2023 at 12:03 PM Masahiro Yamada <[email protected]> wrote:
> >
> > Commit 95ac9b3b585d ("menuconfig: Assign jump keys per-page instead
> > of globally") injects a lot of hacks to the bottom of the textbox
> > infrastructure.
> >
> > I reverted many of them without changing the behavior. (almost)
> > Now, the key markers are inserted when constructing the search result
> > instead of updating the text buffer on-the-fly.
> >
> > The buffer passed to the textbox got back to a constant string.
> > The ugly casts from (const char *) to (char *) went away.
> >
> > A disadvantage is that the same key numbers might be diplayed multiple
> > times in the dialog if you use a huge window (but I believe it is
> > unlikely to happen).
> >
> > Signed-off-by: Masahiro Yamada <[email protected]>
> > ---
> >
> > scripts/kconfig/lkc.h | 1 +
> > scripts/kconfig/lxdialog/dialog.h | 10 ++--
> > scripts/kconfig/lxdialog/textbox.c | 68 +++++++++--------------
> > scripts/kconfig/mconf.c | 86 +++++++++++++++++-------------
> > scripts/kconfig/menu.c | 22 ++++++--
> > 5 files changed, 97 insertions(+), 90 deletions(-)
> >
> > diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
> > index e7118d62a45f..d5c27180ce91 100644
> > --- a/scripts/kconfig/lkc.h
> > +++ b/scripts/kconfig/lkc.h
> > @@ -101,6 +101,7 @@ const char *menu_get_prompt(struct menu *menu);
> > struct menu *menu_get_parent_menu(struct menu *menu);
> > bool menu_has_help(struct menu *menu);
> > const char *menu_get_help(struct menu *menu);
> > +int get_jump_key(void);
> > struct gstr get_relations_str(struct symbol **sym_arr, struct list_head *head);
> > void menu_get_ext_help(struct menu *menu, struct gstr *help);
> >
> > diff --git a/scripts/kconfig/lxdialog/dialog.h b/scripts/kconfig/lxdialog/dialog.h
> > index 347daf25fdc8..cd1b59c24b21 100644
> > --- a/scripts/kconfig/lxdialog/dialog.h
> > +++ b/scripts/kconfig/lxdialog/dialog.h
> > @@ -196,13 +196,9 @@ int first_alpha(const char *string, const char *exempt);
> > int dialog_yesno(const char *title, const char *prompt, int height, int width);
> > int dialog_msgbox(const char *title, const char *prompt, int height,
> > int width, int pause);
> > -
> > -
> > -typedef void (*update_text_fn)(char *buf, size_t start, size_t end, void
> > - *_data);
> > -int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > - int initial_width, int *keys, int *_vscroll, int *_hscroll,
> > - update_text_fn update_text, void *data);
> > +int dialog_textbox(const char *title, const char *tbuf, int initial_height,
> > + int initial_width, int *_vscroll, int *_hscroll,
> > + int (*extra_key_cb)(int, int, int, void *), void *data);
> > int dialog_menu(const char *title, const char *prompt,
> > const void *selected, int *s_scroll);
> > int dialog_checklist(const char *title, const char *prompt, int height,
> > diff --git a/scripts/kconfig/lxdialog/textbox.c b/scripts/kconfig/lxdialog/textbox.c
> > index bc4d4fb1dc75..e6cd7bb83746 100644
> > --- a/scripts/kconfig/lxdialog/textbox.c
> > +++ b/scripts/kconfig/lxdialog/textbox.c
> > @@ -10,8 +10,8 @@
> >
> > static int hscroll;
> > static int begin_reached, end_reached, page_length;
> > -static char *buf;
> > -static char *page;
> > +static const char *buf, *page;
> > +static int start, end;
> >
> > /*
> > * Go back 'n' lines in text. Called by dialog_textbox().
> > @@ -98,21 +98,10 @@ static void print_line(WINDOW *win, int row, int width)
> > /*
> > * Print a new page of text.
> > */
> > -static void print_page(WINDOW *win, int height, int width, update_text_fn
> > - update_text, void *data)
> > +static void print_page(WINDOW *win, int height, int width)
> > {
> > int i, passed_end = 0;
> >
> > - if (update_text) {
> > - char *end;
> > -
> > - for (i = 0; i < height; i++)
> > - get_line();
> > - end = page;
> > - back_lines(height);
> > - update_text(buf, page - buf, end - buf, data);
> > - }
> > -
> > page_length = 0;
> > for (i = 0; i < height; i++) {
> > print_line(win, i, width);
> > @@ -142,24 +131,26 @@ static void print_position(WINDOW *win)
> > * refresh window content
> > */
> > static void refresh_text_box(WINDOW *dialog, WINDOW *box, int boxh, int boxw,
> > - int cur_y, int cur_x, update_text_fn update_text,
> > - void *data)
> > + int cur_y, int cur_x)
>
> The change for refresh_text_box is very large.
> Is there an easy way to split the change of `refresh_text_box` and
> everything else
> while still maintaining bisectability?


I do not think the change is large or complicated.

It just stopped passing down 'update_text' and 'data'.
The same pattern.

The point is this is revert of 95ac9b3b585d

The revert should not be split.


> > @@ -351,11 +333,9 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > on_key_resize();
> > goto do_resize;
> > default:
> > - for (i = 0; keys[i]; i++) {
> > - if (key == keys[i]) {
> > - done = true;
> > - break;
> > - }
> > + if (extra_key_cb(key, start, end, data)) {
>
> `extra_key_cb` is null when not used, on the help page this will segfault.


Thanks.
I will fix it.




> > diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> > index b90fff833588..5578b8bc8a23 100644
> > --- a/scripts/kconfig/menu.c
> > +++ b/scripts/kconfig/menu.c
> > @@ -701,6 +701,11 @@ static void get_dep_str(struct gstr *r, struct expr *expr, const char *prefix)
> > }
> > }
> >
> > +int __attribute__((weak)) get_jump_key(void)
>
> This seems like a non-optimal solution, otherwise fine.


Do you have a better idea?





--
Best Regards
Masahiro Yamada

2023-07-02 19:39:21

by Jesse T

[permalink] [raw]
Subject: Re: [PATCH 1/2] kconfig: menuconfig: simplify global jump key assignment

On Thu, Jun 29, 2023 at 12:03 PM Masahiro Yamada <[email protected]> wrote:
>
> Commit 95ac9b3b585d ("menuconfig: Assign jump keys per-page instead
> of globally") injects a lot of hacks to the bottom of the textbox
> infrastructure.
>
> I reverted many of them without changing the behavior. (almost)
> Now, the key markers are inserted when constructing the search result
> instead of updating the text buffer on-the-fly.
>
> The buffer passed to the textbox got back to a constant string.
> The ugly casts from (const char *) to (char *) went away.
>
> A disadvantage is that the same key numbers might be diplayed multiple
> times in the dialog if you use a huge window (but I believe it is
> unlikely to happen).
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
> scripts/kconfig/lkc.h | 1 +
> scripts/kconfig/lxdialog/dialog.h | 10 ++--
> scripts/kconfig/lxdialog/textbox.c | 68 +++++++++--------------
> scripts/kconfig/mconf.c | 86 +++++++++++++++++-------------
> scripts/kconfig/menu.c | 22 ++++++--
> 5 files changed, 97 insertions(+), 90 deletions(-)
>
> diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
> index e7118d62a45f..d5c27180ce91 100644
> --- a/scripts/kconfig/lkc.h
> +++ b/scripts/kconfig/lkc.h
> @@ -101,6 +101,7 @@ const char *menu_get_prompt(struct menu *menu);
> struct menu *menu_get_parent_menu(struct menu *menu);
> bool menu_has_help(struct menu *menu);
> const char *menu_get_help(struct menu *menu);
> +int get_jump_key(void);
> struct gstr get_relations_str(struct symbol **sym_arr, struct list_head *head);
> void menu_get_ext_help(struct menu *menu, struct gstr *help);
>
> diff --git a/scripts/kconfig/lxdialog/dialog.h b/scripts/kconfig/lxdialog/dialog.h
> index 347daf25fdc8..cd1b59c24b21 100644
> --- a/scripts/kconfig/lxdialog/dialog.h
> +++ b/scripts/kconfig/lxdialog/dialog.h
> @@ -196,13 +196,9 @@ int first_alpha(const char *string, const char *exempt);
> int dialog_yesno(const char *title, const char *prompt, int height, int width);
> int dialog_msgbox(const char *title, const char *prompt, int height,
> int width, int pause);
> -
> -
> -typedef void (*update_text_fn)(char *buf, size_t start, size_t end, void
> - *_data);
> -int dialog_textbox(const char *title, char *tbuf, int initial_height,
> - int initial_width, int *keys, int *_vscroll, int *_hscroll,
> - update_text_fn update_text, void *data);
> +int dialog_textbox(const char *title, const char *tbuf, int initial_height,
> + int initial_width, int *_vscroll, int *_hscroll,
> + int (*extra_key_cb)(int, int, int, void *), void *data);
> int dialog_menu(const char *title, const char *prompt,
> const void *selected, int *s_scroll);
> int dialog_checklist(const char *title, const char *prompt, int height,
> diff --git a/scripts/kconfig/lxdialog/textbox.c b/scripts/kconfig/lxdialog/textbox.c
> index bc4d4fb1dc75..e6cd7bb83746 100644
> --- a/scripts/kconfig/lxdialog/textbox.c
> +++ b/scripts/kconfig/lxdialog/textbox.c
> @@ -10,8 +10,8 @@
>
> static int hscroll;
> static int begin_reached, end_reached, page_length;
> -static char *buf;
> -static char *page;
> +static const char *buf, *page;
> +static int start, end;
>
> /*
> * Go back 'n' lines in text. Called by dialog_textbox().
> @@ -98,21 +98,10 @@ static void print_line(WINDOW *win, int row, int width)
> /*
> * Print a new page of text.
> */
> -static void print_page(WINDOW *win, int height, int width, update_text_fn
> - update_text, void *data)
> +static void print_page(WINDOW *win, int height, int width)
> {
> int i, passed_end = 0;
>
> - if (update_text) {
> - char *end;
> -
> - for (i = 0; i < height; i++)
> - get_line();
> - end = page;
> - back_lines(height);
> - update_text(buf, page - buf, end - buf, data);
> - }
> -
> page_length = 0;
> for (i = 0; i < height; i++) {
> print_line(win, i, width);
> @@ -142,24 +131,26 @@ static void print_position(WINDOW *win)
> * refresh window content
> */
> static void refresh_text_box(WINDOW *dialog, WINDOW *box, int boxh, int boxw,
> - int cur_y, int cur_x, update_text_fn update_text,
> - void *data)
> + int cur_y, int cur_x)
> {
> - print_page(box, boxh, boxw, update_text, data);
> + start = page - buf;
> +
> + print_page(box, boxh, boxw);
> print_position(dialog);
> wmove(dialog, cur_y, cur_x); /* Restore cursor position */
> wrefresh(dialog);
> +
> + end = page - buf;
> }
>
> /*
> * Display text from a file in a dialog box.
> *
> * keys is a null-terminated array
> - * update_text() may not add or remove any '\n' or '\0' in tbuf
> */
> -int dialog_textbox(const char *title, char *tbuf, int initial_height,
> - int initial_width, int *keys, int *_vscroll, int *_hscroll,
> - update_text_fn update_text, void *data)
> +int dialog_textbox(const char *title, const char *tbuf, int initial_height,
> + int initial_width, int *_vscroll, int *_hscroll,
> + int (*extra_key_cb)(int, int, int, void *), void *data)
> {
> int i, x, y, cur_x, cur_y, key = 0;
> int height, width, boxh, boxw;
> @@ -239,8 +230,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
>
> /* Print first page of text */
> attr_clear(box, boxh, boxw, dlg.dialog.atr);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x, update_text,
> - data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
>
> while (!done) {
> key = wgetch(dialog);
> @@ -259,8 +249,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> begin_reached = 1;
> page = buf;
> refresh_text_box(dialog, box, boxh, boxw,
> - cur_y, cur_x, update_text,
> - data);
> + cur_y, cur_x);
> }
> break;
> case 'G': /* Last page */
> @@ -270,8 +259,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> /* point to last char in buf */
> page = buf + strlen(buf);
> back_lines(boxh);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case 'K': /* Previous line */
> case 'k':
> @@ -280,8 +268,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> break;
>
> back_lines(page_length + 1);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case 'B': /* Previous page */
> case 'b':
> @@ -290,8 +277,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> if (begin_reached)
> break;
> back_lines(page_length + boxh);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case 'J': /* Next line */
> case 'j':
> @@ -300,8 +286,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> break;
>
> back_lines(page_length - 1);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case KEY_NPAGE: /* Next page */
> case ' ':
> @@ -310,8 +295,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> break;
>
> begin_reached = 0;
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case '0': /* Beginning of line */
> case 'H': /* Scroll left */
> @@ -326,8 +310,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> hscroll--;
> /* Reprint current page to scroll horizontally */
> back_lines(page_length);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case 'L': /* Scroll right */
> case 'l':
> @@ -337,8 +320,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> hscroll++;
> /* Reprint current page to scroll horizontally */
> back_lines(page_length);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case KEY_ESC:
> if (on_key_esc(dialog) == KEY_ESC)
> @@ -351,11 +333,9 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> on_key_resize();
> goto do_resize;
> default:
> - for (i = 0; keys[i]; i++) {
> - if (key == keys[i]) {
> - done = true;
> - break;
> - }
> + if (extra_key_cb(key, start, end, data)) {
> + done = true;
> + break;
> }
> }
> }
> diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
> index 53d8834d12fe..7adfd6537279 100644
> --- a/scripts/kconfig/mconf.c
> +++ b/scripts/kconfig/mconf.c
> @@ -288,6 +288,7 @@ static int single_menu_mode;
> static int show_all_options;
> static int save_and_exit;
> static int silent;
> +static int jump_key;
>
> static void conf(struct menu *menu, struct menu *active_menu);
>
> @@ -348,19 +349,19 @@ static void reset_subtitle(void)
> set_dialog_subtitles(subtitles);
> }
>
> -static int show_textbox_ext(const char *title, char *text, int r, int c, int
> - *keys, int *vscroll, int *hscroll, update_text_fn
> - update_text, void *data)
> +static int show_textbox_ext(const char *title, const char *text, int r, int c,
> + int *vscroll, int *hscroll,
> + int (*extra_key_cb)(int, int, int, void *),
> + void *data)
> {
> dialog_clear();
> - return dialog_textbox(title, text, r, c, keys, vscroll, hscroll,
> - update_text, data);
> + return dialog_textbox(title, text, r, c, vscroll, hscroll,
> + extra_key_cb, data);
> }
>
> static void show_textbox(const char *title, const char *text, int r, int c)
> {
> - show_textbox_ext(title, (char *) text, r, c, (int []) {0}, NULL, NULL,
> - NULL, NULL);
> + show_textbox_ext(title, text, r, c, NULL, NULL, NULL, NULL);
> }
>
> static void show_helptext(const char *title, const char *text)
> @@ -381,35 +382,51 @@ static void show_help(struct menu *menu)
>
> struct search_data {
> struct list_head *head;
> - struct menu **targets;
> - int *keys;
> + struct menu *target;
> };
>
> -static void update_text(char *buf, size_t start, size_t end, void *_data)
> +static int next_key(int key)
> +{
> + key++;
> +
> + if (key > '9')
> + key = '1';
> +
> + return key;
> +}
> +
> +static int handle_search_keys(int key, int start, int end, void *_data)
> {
> struct search_data *data = _data;
> struct jump_key *pos;
> - int k = 0;
> +
> + if (key < '1' || key > '9')
> + return 0;
>
> list_for_each_entry(pos, data->head, entries) {
> - if (pos->offset >= start && pos->offset < end) {
> - char header[4];
> + if (pos->offset >= start) {
> + if (pos->offset >= end)
> + break;
>
> - if (k < JUMP_NB) {
> - int key = '0' + (pos->index % JUMP_NB) + 1;
> -
> - sprintf(header, "(%c)", key);
> - data->keys[k] = key;
> - data->targets[k] = pos->target;
> - k++;
> - } else {
> - sprintf(header, " ");
> + if (key == '1' + (pos->index % JUMP_NB)) {
> + data->target = pos->target;
> + return 1;
> }
> -
> - memcpy(buf + pos->offset, header, sizeof(header) - 1);
> }
> }
> - data->keys[k] = 0;
> +
> + return 0;
> +}
> +
> +int get_jump_key(void)
> +{
> + int cur_key;
> +
> + cur_key = jump_key;

There should also be a check to see if jump_key is valid.
jump_key can be set to 0 and will have weird effects.

> +
> + jump_key = next_key(cur_key);
> +
> + return cur_key;
> }
>
> static void search_conf(void)
> @@ -456,26 +473,23 @@ static void search_conf(void)
> sym_arr = sym_re_search(dialog_input);
> do {
> LIST_HEAD(head);
> - struct menu *targets[JUMP_NB];
> - int keys[JUMP_NB + 1], i;
> struct search_data data = {
> .head = &head,

.target = NULL,
We check if target is null later on, we can make it more explicit

> - .targets = targets,
> - .keys = keys,
> };
> struct jump_key *pos, *tmp;
>
> + jump_key = '1';
> res = get_relations_str(sym_arr, &head);
> set_subtitle();
> dres = show_textbox_ext("Search Results", str_get(&res), 0, 0,
> - keys, &vscroll, &hscroll, &update_text,
> - &data);
> + &vscroll, &hscroll,
> + handle_search_keys, &data);
> again = false;
> - for (i = 0; i < JUMP_NB && keys[i]; i++)
> - if (dres == keys[i]) {
> - conf(targets[i]->parent, targets[i]);
> - again = true;
> - }
> + if (dres >= '1' && dres <= '9') {
> + assert(data.target != NULL);
> + conf(data.target->parent, data.target);
> + again = true;
> + }
> str_free(&res);
> list_for_each_entry_safe(pos, tmp, &head, entries)
> free(pos);

while here the formatting on the above line is one extra tab indented.


> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index b90fff833588..5578b8bc8a23 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -701,6 +701,11 @@ static void get_dep_str(struct gstr *r, struct expr *expr, const char *prefix)
> }
> }
>
> > > +int __attribute__((weak)) get_jump_key(void)
> > This seems like a non-optimal solution, otherwise fine.
> Do you have a better idea?

I do not.

> +{
> + return -1;
> +}
> +
> static void get_prompt_str(struct gstr *r, struct property *prop,
> struct list_head *head)
> {
> @@ -743,11 +748,22 @@ static void get_prompt_str(struct gstr *r, struct property *prop,
> }
>
> str_printf(r, " Location:\n");
> - for (j = 4; --i >= 0; j += 2) {
> + for (j = 0; --i >= 0; j++) {
> + int jk = -1;
> + int indent = 2 * j + 4;
> +
> menu = submenu[i];
> - if (jump && menu == location)
> + if (jump && menu == location) {
> jump->offset = strlen(r->s);
> - str_printf(r, "%*c-> %s", j, ' ', menu_get_prompt(menu));
> + jk = get_jump_key();
> + }
> +
> + if (jk >= 0) {
> + str_printf(r, "(%c)", jk);
> + indent -= 3;
> + }
> +
> + str_printf(r, "%*c-> %s", indent, ' ', menu_get_prompt(menu));
> if (menu->sym) {
> str_printf(r, " (%s [=%s])", menu->sym->name ?
> menu->sym->name : "<choice>",
> --
> 2.39.2
>

2023-07-02 21:13:09

by Jesse T

[permalink] [raw]
Subject: Re: [PATCH 1/2] kconfig: menuconfig: simplify global jump key assignment

On Sun, Jul 2, 2023 at 3:32 PM Jesse T <[email protected]> wrote:
>
> On Thu, Jun 29, 2023 at 12:03 PM Masahiro Yamada <[email protected]> wrote:
> >
> > Commit 95ac9b3b585d ("menuconfig: Assign jump keys per-page instead
> > of globally") injects a lot of hacks to the bottom of the textbox
> > infrastructure.
> >
> > I reverted many of them without changing the behavior. (almost)
> > Now, the key markers are inserted when constructing the search result
> > instead of updating the text buffer on-the-fly.
> >
> > The buffer passed to the textbox got back to a constant string.
> > The ugly casts from (const char *) to (char *) went away.
> >
> > A disadvantage is that the same key numbers might be diplayed multiple
> > times in the dialog if you use a huge window (but I believe it is
> > unlikely to happen).
> >
> > Signed-off-by: Masahiro Yamada <[email protected]>
> > ---
> >
> > scripts/kconfig/lkc.h | 1 +
> > scripts/kconfig/lxdialog/dialog.h | 10 ++--
> > scripts/kconfig/lxdialog/textbox.c | 68 +++++++++--------------
> > scripts/kconfig/mconf.c | 86 +++++++++++++++++-------------
> > scripts/kconfig/menu.c | 22 ++++++--
> > 5 files changed, 97 insertions(+), 90 deletions(-)
> >
> > diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
> > index e7118d62a45f..d5c27180ce91 100644
> > --- a/scripts/kconfig/lkc.h
> > +++ b/scripts/kconfig/lkc.h
> > @@ -101,6 +101,7 @@ const char *menu_get_prompt(struct menu *menu);
> > struct menu *menu_get_parent_menu(struct menu *menu);
> > bool menu_has_help(struct menu *menu);
> > const char *menu_get_help(struct menu *menu);
> > +int get_jump_key(void);
> > struct gstr get_relations_str(struct symbol **sym_arr, struct list_head *head);
> > void menu_get_ext_help(struct menu *menu, struct gstr *help);
> >
> > diff --git a/scripts/kconfig/lxdialog/dialog.h b/scripts/kconfig/lxdialog/dialog.h
> > index 347daf25fdc8..cd1b59c24b21 100644
> > --- a/scripts/kconfig/lxdialog/dialog.h
> > +++ b/scripts/kconfig/lxdialog/dialog.h
> > @@ -196,13 +196,9 @@ int first_alpha(const char *string, const char *exempt);
> > int dialog_yesno(const char *title, const char *prompt, int height, int width);
> > int dialog_msgbox(const char *title, const char *prompt, int height,
> > int width, int pause);
> > -
> > -
> > -typedef void (*update_text_fn)(char *buf, size_t start, size_t end, void
> > - *_data);
> > -int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > - int initial_width, int *keys, int *_vscroll, int *_hscroll,
> > - update_text_fn update_text, void *data);
> > +int dialog_textbox(const char *title, const char *tbuf, int initial_height,
> > + int initial_width, int *_vscroll, int *_hscroll,
> > + int (*extra_key_cb)(int, int, int, void *), void *data);
> > int dialog_menu(const char *title, const char *prompt,
> > const void *selected, int *s_scroll);
> > int dialog_checklist(const char *title, const char *prompt, int height,
> > diff --git a/scripts/kconfig/lxdialog/textbox.c b/scripts/kconfig/lxdialog/textbox.c
> > index bc4d4fb1dc75..e6cd7bb83746 100644
> > --- a/scripts/kconfig/lxdialog/textbox.c
> > +++ b/scripts/kconfig/lxdialog/textbox.c
> > @@ -10,8 +10,8 @@
> >
> > static int hscroll;
> > static int begin_reached, end_reached, page_length;
> > -static char *buf;
> > -static char *page;
> > +static const char *buf, *page;
> > +static int start, end;
> >
> > /*
> > * Go back 'n' lines in text. Called by dialog_textbox().
> > @@ -98,21 +98,10 @@ static void print_line(WINDOW *win, int row, int width)
> > /*
> > * Print a new page of text.
> > */
> > -static void print_page(WINDOW *win, int height, int width, update_text_fn
> > - update_text, void *data)
> > +static void print_page(WINDOW *win, int height, int width)
> > {
> > int i, passed_end = 0;
> >
> > - if (update_text) {
> > - char *end;
> > -
> > - for (i = 0; i < height; i++)
> > - get_line();
> > - end = page;
> > - back_lines(height);
> > - update_text(buf, page - buf, end - buf, data);
> > - }
> > -
> > page_length = 0;
> > for (i = 0; i < height; i++) {
> > print_line(win, i, width);
> > @@ -142,24 +131,26 @@ static void print_position(WINDOW *win)
> > * refresh window content
> > */
> > static void refresh_text_box(WINDOW *dialog, WINDOW *box, int boxh, int boxw,
> > - int cur_y, int cur_x, update_text_fn update_text,
> > - void *data)
> > + int cur_y, int cur_x)
> > {
> > - print_page(box, boxh, boxw, update_text, data);
> > + start = page - buf;
> > +
> > + print_page(box, boxh, boxw);
> > print_position(dialog);
> > wmove(dialog, cur_y, cur_x); /* Restore cursor position */
> > wrefresh(dialog);
> > +
> > + end = page - buf;
> > }
> >
> > /*
> > * Display text from a file in a dialog box.
> > *
> > * keys is a null-terminated array
> > - * update_text() may not add or remove any '\n' or '\0' in tbuf
> > */
> > -int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > - int initial_width, int *keys, int *_vscroll, int *_hscroll,
> > - update_text_fn update_text, void *data)
> > +int dialog_textbox(const char *title, const char *tbuf, int initial_height,
> > + int initial_width, int *_vscroll, int *_hscroll,
> > + int (*extra_key_cb)(int, int, int, void *), void *data)
> > {
> > int i, x, y, cur_x, cur_y, key = 0;
> > int height, width, boxh, boxw;
> > @@ -239,8 +230,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> >
> > /* Print first page of text */
> > attr_clear(box, boxh, boxw, dlg.dialog.atr);
> > - refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x, update_text,
> > - data);
> > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> >
> > while (!done) {
> > key = wgetch(dialog);
> > @@ -259,8 +249,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > begin_reached = 1;
> > page = buf;
> > refresh_text_box(dialog, box, boxh, boxw,
> > - cur_y, cur_x, update_text,
> > - data);
> > + cur_y, cur_x);
> > }
> > break;
> > case 'G': /* Last page */
> > @@ -270,8 +259,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > /* point to last char in buf */
> > page = buf + strlen(buf);
> > back_lines(boxh);
> > - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> > - cur_x, update_text, data);
> > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > break;
> > case 'K': /* Previous line */
> > case 'k':
> > @@ -280,8 +268,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > break;
> >
> > back_lines(page_length + 1);
> > - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> > - cur_x, update_text, data);
> > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > break;
> > case 'B': /* Previous page */
> > case 'b':
> > @@ -290,8 +277,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > if (begin_reached)
> > break;
> > back_lines(page_length + boxh);
> > - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> > - cur_x, update_text, data);
> > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > break;
> > case 'J': /* Next line */
> > case 'j':
> > @@ -300,8 +286,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > break;
> >
> > back_lines(page_length - 1);
> > - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> > - cur_x, update_text, data);
> > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > break;
> > case KEY_NPAGE: /* Next page */
> > case ' ':
> > @@ -310,8 +295,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > break;
> >
> > begin_reached = 0;
> > - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> > - cur_x, update_text, data);
> > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > break;
> > case '0': /* Beginning of line */
> > case 'H': /* Scroll left */
> > @@ -326,8 +310,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > hscroll--;
> > /* Reprint current page to scroll horizontally */
> > back_lines(page_length);
> > - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> > - cur_x, update_text, data);
> > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > break;
> > case 'L': /* Scroll right */
> > case 'l':
> > @@ -337,8 +320,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > hscroll++;
> > /* Reprint current page to scroll horizontally */
> > back_lines(page_length);
> > - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> > - cur_x, update_text, data);
> > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > break;
> > case KEY_ESC:
> > if (on_key_esc(dialog) == KEY_ESC)
> > @@ -351,11 +333,9 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > on_key_resize();
> > goto do_resize;
> > default:
> > - for (i = 0; keys[i]; i++) {
> > - if (key == keys[i]) {
> > - done = true;
> > - break;
> > - }
> > + if (extra_key_cb(key, start, end, data)) {
> > + done = true;
> > + break;
> > }
> > }
> > }
> > diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
> > index 53d8834d12fe..7adfd6537279 100644
> > --- a/scripts/kconfig/mconf.c
> > +++ b/scripts/kconfig/mconf.c
> > @@ -288,6 +288,7 @@ static int single_menu_mode;
> > static int show_all_options;
> > static int save_and_exit;
> > static int silent;
> > +static int jump_key;
> >
> > static void conf(struct menu *menu, struct menu *active_menu);
> >
> > @@ -348,19 +349,19 @@ static void reset_subtitle(void)
> > set_dialog_subtitles(subtitles);
> > }
> >
> > -static int show_textbox_ext(const char *title, char *text, int r, int c, int
> > - *keys, int *vscroll, int *hscroll, update_text_fn
> > - update_text, void *data)
> > +static int show_textbox_ext(const char *title, const char *text, int r, int c,
> > + int *vscroll, int *hscroll,
> > + int (*extra_key_cb)(int, int, int, void *),
> > + void *data)
> > {
> > dialog_clear();
> > - return dialog_textbox(title, text, r, c, keys, vscroll, hscroll,
> > - update_text, data);
> > + return dialog_textbox(title, text, r, c, vscroll, hscroll,
> > + extra_key_cb, data);
> > }
> >
> > static void show_textbox(const char *title, const char *text, int r, int c)
> > {
> > - show_textbox_ext(title, (char *) text, r, c, (int []) {0}, NULL, NULL,
> > - NULL, NULL);
> > + show_textbox_ext(title, text, r, c, NULL, NULL, NULL, NULL);
> > }
> >
> > static void show_helptext(const char *title, const char *text)
> > @@ -381,35 +382,51 @@ static void show_help(struct menu *menu)
> >
> > struct search_data {
> > struct list_head *head;
> > - struct menu **targets;
> > - int *keys;
> > + struct menu *target;
> > };
> >
> > -static void update_text(char *buf, size_t start, size_t end, void *_data)
> > +static int next_key(int key)
> > +{
> > + key++;
> > +
> > + if (key > '9')
> > + key = '1';
> > +
> > + return key;
> > +}
> > +
> > +static int handle_search_keys(int key, int start, int end, void *_data)
> > {
> > struct search_data *data = _data;
> > struct jump_key *pos;
> > - int k = 0;
> > +
> > + if (key < '1' || key > '9')
> > + return 0;
> >
> > list_for_each_entry(pos, data->head, entries) {
> > - if (pos->offset >= start && pos->offset < end) {
Sorry forgot to mention this, but start and end should be size_t.
You get -Wsign-compare.

> > - char header[4];
> > + if (pos->offset >= start) {
> > + if (pos->offset >= end)
> > + break;
> >
> > - if (k < JUMP_NB) {
> > - int key = '0' + (pos->index % JUMP_NB) + 1;
> > -
> > - sprintf(header, "(%c)", key);
> > - data->keys[k] = key;
> > - data->targets[k] = pos->target;
> > - k++;
> > - } else {
> > - sprintf(header, " ");
> > + if (key == '1' + (pos->index % JUMP_NB)) {
> > + data->target = pos->target;
> > + return 1;
> > }
> > -
> > - memcpy(buf + pos->offset, header, sizeof(header) - 1);
> > }
> > }
> > - data->keys[k] = 0;
> > +
> > + return 0;
> > +}
> > +
> > +int get_jump_key(void)
> > +{
> > + int cur_key;
> > +
> > + cur_key = jump_key;
>
> There should also be a check to see if jump_key is valid.
> jump_key can be set to 0 and will have weird effects.
>
> > +
> > + jump_key = next_key(cur_key);
> > +
> > + return cur_key;
> > }
> >
> > static void search_conf(void)
> > @@ -456,26 +473,23 @@ static void search_conf(void)
> > sym_arr = sym_re_search(dialog_input);
> > do {
> > LIST_HEAD(head);
> > - struct menu *targets[JUMP_NB];
> > - int keys[JUMP_NB + 1], i;
> > struct search_data data = {
> > .head = &head,
>
> .target = NULL,
> We check if target is null later on, we can make it more explicit
>
> > - .targets = targets,
> > - .keys = keys,
> > };
> > struct jump_key *pos, *tmp;
> >
> > + jump_key = '1';
> > res = get_relations_str(sym_arr, &head);
> > set_subtitle();
> > dres = show_textbox_ext("Search Results", str_get(&res), 0, 0,
> > - keys, &vscroll, &hscroll, &update_text,
> > - &data);
> > + &vscroll, &hscroll,
> > + handle_search_keys, &data);
> > again = false;
> > - for (i = 0; i < JUMP_NB && keys[i]; i++)
> > - if (dres == keys[i]) {
> > - conf(targets[i]->parent, targets[i]);
> > - again = true;
> > - }
> > + if (dres >= '1' && dres <= '9') {
> > + assert(data.target != NULL);
> > + conf(data.target->parent, data.target);
> > + again = true;
> > + }
> > str_free(&res);
> > list_for_each_entry_safe(pos, tmp, &head, entries)
> > free(pos);
>
> while here the formatting on the above line is one extra tab indented.
>
>
> > diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> > index b90fff833588..5578b8bc8a23 100644
> > --- a/scripts/kconfig/menu.c
> > +++ b/scripts/kconfig/menu.c
> > @@ -701,6 +701,11 @@ static void get_dep_str(struct gstr *r, struct expr *expr, const char *prefix)
> > }
> > }
> >
> > > > +int __attribute__((weak)) get_jump_key(void)
> > > This seems like a non-optimal solution, otherwise fine.
> > Do you have a better idea?
>
> I do not.
>
> > +{
> > + return -1;
> > +}
> > +
> > static void get_prompt_str(struct gstr *r, struct property *prop,
> > struct list_head *head)
> > {
> > @@ -743,11 +748,22 @@ static void get_prompt_str(struct gstr *r, struct property *prop,
> > }
> >
> > str_printf(r, " Location:\n");
> > - for (j = 4; --i >= 0; j += 2) {
> > + for (j = 0; --i >= 0; j++) {
> > + int jk = -1;
> > + int indent = 2 * j + 4;
> > +
> > menu = submenu[i];
> > - if (jump && menu == location)
> > + if (jump && menu == location) {
> > jump->offset = strlen(r->s);
> > - str_printf(r, "%*c-> %s", j, ' ', menu_get_prompt(menu));
> > + jk = get_jump_key();
> > + }
> > +
> > + if (jk >= 0) {
> > + str_printf(r, "(%c)", jk);
> > + indent -= 3;
> > + }
> > +
> > + str_printf(r, "%*c-> %s", indent, ' ', menu_get_prompt(menu));
> > if (menu->sym) {
> > str_printf(r, " (%s [=%s])", menu->sym->name ?
> > menu->sym->name : "<choice>",
> > --
> > 2.39.2
> >

2023-07-12 18:03:27

by Jesse T

[permalink] [raw]
Subject: Re: [PATCH 1/2] kconfig: menuconfig: simplify global jump key assignment

On Sun, Jul 2, 2023 at 4:26 PM Jesse T <[email protected]> wrote:
>
> On Sun, Jul 2, 2023 at 3:32 PM Jesse T <[email protected]> wrote:
> >
> > On Thu, Jun 29, 2023 at 12:03 PM Masahiro Yamada <[email protected]> wrote:
> > >
> > > Commit 95ac9b3b585d ("menuconfig: Assign jump keys per-page instead
> > > of globally") injects a lot of hacks to the bottom of the textbox
> > > infrastructure.
> > >
> > > I reverted many of them without changing the behavior. (almost)
> > > Now, the key markers are inserted when constructing the search result
> > > instead of updating the text buffer on-the-fly.
> > >
> > > The buffer passed to the textbox got back to a constant string.
> > > The ugly casts from (const char *) to (char *) went away.
> > >
> > > A disadvantage is that the same key numbers might be diplayed multiple
> > > times in the dialog if you use a huge window (but I believe it is
> > > unlikely to happen).
> > >
> > > Signed-off-by: Masahiro Yamada <[email protected]>
> > > ---
> > >
> > > scripts/kconfig/lkc.h | 1 +
> > > scripts/kconfig/lxdialog/dialog.h | 10 ++--
> > > scripts/kconfig/lxdialog/textbox.c | 68 +++++++++--------------
> > > scripts/kconfig/mconf.c | 86 +++++++++++++++++-------------
> > > scripts/kconfig/menu.c | 22 ++++++--
> > > 5 files changed, 97 insertions(+), 90 deletions(-)
> > >
> > > diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
> > > index e7118d62a45f..d5c27180ce91 100644
> > > --- a/scripts/kconfig/lkc.h
> > > +++ b/scripts/kconfig/lkc.h
> > > @@ -101,6 +101,7 @@ const char *menu_get_prompt(struct menu *menu);
> > > struct menu *menu_get_parent_menu(struct menu *menu);
> > > bool menu_has_help(struct menu *menu);
> > > const char *menu_get_help(struct menu *menu);
> > > +int get_jump_key(void);
> > > struct gstr get_relations_str(struct symbol **sym_arr, struct list_head *head);
> > > void menu_get_ext_help(struct menu *menu, struct gstr *help);
> > >
> > > diff --git a/scripts/kconfig/lxdialog/dialog.h b/scripts/kconfig/lxdialog/dialog.h
> > > index 347daf25fdc8..cd1b59c24b21 100644
> > > --- a/scripts/kconfig/lxdialog/dialog.h
> > > +++ b/scripts/kconfig/lxdialog/dialog.h
> > > @@ -196,13 +196,9 @@ int first_alpha(const char *string, const char *exempt);
> > > int dialog_yesno(const char *title, const char *prompt, int height, int width);
> > > int dialog_msgbox(const char *title, const char *prompt, int height,
> > > int width, int pause);
> > > -
> > > -
> > > -typedef void (*update_text_fn)(char *buf, size_t start, size_t end, void
> > > - *_data);
> > > -int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > > - int initial_width, int *keys, int *_vscroll, int *_hscroll,
> > > - update_text_fn update_text, void *data);
> > > +int dialog_textbox(const char *title, const char *tbuf, int initial_height,
> > > + int initial_width, int *_vscroll, int *_hscroll,
> > > + int (*extra_key_cb)(int, int, int, void *), void *data);
> > > int dialog_menu(const char *title, const char *prompt,
> > > const void *selected, int *s_scroll);
> > > int dialog_checklist(const char *title, const char *prompt, int height,
> > > diff --git a/scripts/kconfig/lxdialog/textbox.c b/scripts/kconfig/lxdialog/textbox.c
> > > index bc4d4fb1dc75..e6cd7bb83746 100644
> > > --- a/scripts/kconfig/lxdialog/textbox.c
> > > +++ b/scripts/kconfig/lxdialog/textbox.c
> > > @@ -10,8 +10,8 @@
> > >
> > > static int hscroll;
> > > static int begin_reached, end_reached, page_length;
> > > -static char *buf;
> > > -static char *page;
> > > +static const char *buf, *page;
> > > +static int start, end;
> > >
> > > /*
> > > * Go back 'n' lines in text. Called by dialog_textbox().
> > > @@ -98,21 +98,10 @@ static void print_line(WINDOW *win, int row, int width)
> > > /*
> > > * Print a new page of text.
> > > */
> > > -static void print_page(WINDOW *win, int height, int width, update_text_fn
> > > - update_text, void *data)
> > > +static void print_page(WINDOW *win, int height, int width)
> > > {
> > > int i, passed_end = 0;
> > >
> > > - if (update_text) {
> > > - char *end;
> > > -
> > > - for (i = 0; i < height; i++)
> > > - get_line();
> > > - end = page;
> > > - back_lines(height);
> > > - update_text(buf, page - buf, end - buf, data);
> > > - }
> > > -
> > > page_length = 0;
> > > for (i = 0; i < height; i++) {
> > > print_line(win, i, width);
> > > @@ -142,24 +131,26 @@ static void print_position(WINDOW *win)
> > > * refresh window content
> > > */
> > > static void refresh_text_box(WINDOW *dialog, WINDOW *box, int boxh, int boxw,
> > > - int cur_y, int cur_x, update_text_fn update_text,
> > > - void *data)
> > > + int cur_y, int cur_x)
> > > {
> > > - print_page(box, boxh, boxw, update_text, data);
> > > + start = page - buf;
> > > +
> > > + print_page(box, boxh, boxw);
> > > print_position(dialog);
> > > wmove(dialog, cur_y, cur_x); /* Restore cursor position */
> > > wrefresh(dialog);
> > > +
> > > + end = page - buf;
> > > }
> > >
> > > /*
> > > * Display text from a file in a dialog box.
> > > *
> > > * keys is a null-terminated array
> > > - * update_text() may not add or remove any '\n' or '\0' in tbuf
> > > */
> > > -int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > > - int initial_width, int *keys, int *_vscroll, int *_hscroll,
> > > - update_text_fn update_text, void *data)
> > > +int dialog_textbox(const char *title, const char *tbuf, int initial_height,
> > > + int initial_width, int *_vscroll, int *_hscroll,
> > > + int (*extra_key_cb)(int, int, int, void *), void *data)
> > > {
> > > int i, x, y, cur_x, cur_y, key = 0;
> > > int height, width, boxh, boxw;
> > > @@ -239,8 +230,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > >
> > > /* Print first page of text */
> > > attr_clear(box, boxh, boxw, dlg.dialog.atr);
> > > - refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x, update_text,
> > > - data);
> > > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > >
> > > while (!done) {
> > > key = wgetch(dialog);
> > > @@ -259,8 +249,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > > begin_reached = 1;
> > > page = buf;
> > > refresh_text_box(dialog, box, boxh, boxw,
> > > - cur_y, cur_x, update_text,
> > > - data);
> > > + cur_y, cur_x);
> > > }
> > > break;
> > > case 'G': /* Last page */
> > > @@ -270,8 +259,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > > /* point to last char in buf */
> > > page = buf + strlen(buf);
> > > back_lines(boxh);
> > > - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> > > - cur_x, update_text, data);
> > > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > > break;
> > > case 'K': /* Previous line */
> > > case 'k':
> > > @@ -280,8 +268,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > > break;
> > >
> > > back_lines(page_length + 1);
> > > - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> > > - cur_x, update_text, data);
> > > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > > break;
> > > case 'B': /* Previous page */
> > > case 'b':
> > > @@ -290,8 +277,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > > if (begin_reached)
> > > break;
> > > back_lines(page_length + boxh);
> > > - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> > > - cur_x, update_text, data);
> > > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > > break;
> > > case 'J': /* Next line */
> > > case 'j':
> > > @@ -300,8 +286,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > > break;
> > >
> > > back_lines(page_length - 1);
> > > - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> > > - cur_x, update_text, data);
> > > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > > break;
> > > case KEY_NPAGE: /* Next page */
> > > case ' ':
> > > @@ -310,8 +295,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > > break;
> > >
> > > begin_reached = 0;
> > > - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> > > - cur_x, update_text, data);
> > > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > > break;
> > > case '0': /* Beginning of line */
> > > case 'H': /* Scroll left */
> > > @@ -326,8 +310,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > > hscroll--;
> > > /* Reprint current page to scroll horizontally */
> > > back_lines(page_length);
> > > - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> > > - cur_x, update_text, data);
> > > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > > break;
> > > case 'L': /* Scroll right */
> > > case 'l':
> > > @@ -337,8 +320,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > > hscroll++;
> > > /* Reprint current page to scroll horizontally */
> > > back_lines(page_length);
> > > - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> > > - cur_x, update_text, data);
> > > + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> > > break;
> > > case KEY_ESC:
> > > if (on_key_esc(dialog) == KEY_ESC)
> > > @@ -351,11 +333,9 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> > > on_key_resize();
> > > goto do_resize;
> > > default:
> > > - for (i = 0; keys[i]; i++) {
> > > - if (key == keys[i]) {
> > > - done = true;
> > > - break;
> > > - }
> > > + if (extra_key_cb(key, start, end, data)) {
> > > + done = true;
> > > + break;
> > > }
> > > }
> > > }
> > > diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
> > > index 53d8834d12fe..7adfd6537279 100644
> > > --- a/scripts/kconfig/mconf.c
> > > +++ b/scripts/kconfig/mconf.c
> > > @@ -288,6 +288,7 @@ static int single_menu_mode;
> > > static int show_all_options;
> > > static int save_and_exit;
> > > static int silent;
> > > +static int jump_key;
> > >
> > > static void conf(struct menu *menu, struct menu *active_menu);
> > >
> > > @@ -348,19 +349,19 @@ static void reset_subtitle(void)
> > > set_dialog_subtitles(subtitles);
> > > }
> > >
> > > -static int show_textbox_ext(const char *title, char *text, int r, int c, int
> > > - *keys, int *vscroll, int *hscroll, update_text_fn
> > > - update_text, void *data)
> > > +static int show_textbox_ext(const char *title, const char *text, int r, int c,
> > > + int *vscroll, int *hscroll,
> > > + int (*extra_key_cb)(int, int, int, void *),
> > > + void *data)
> > > {
> > > dialog_clear();
> > > - return dialog_textbox(title, text, r, c, keys, vscroll, hscroll,
> > > - update_text, data);
> > > + return dialog_textbox(title, text, r, c, vscroll, hscroll,
> > > + extra_key_cb, data);
> > > }
> > >
> > > static void show_textbox(const char *title, const char *text, int r, int c)
> > > {
> > > - show_textbox_ext(title, (char *) text, r, c, (int []) {0}, NULL, NULL,
> > > - NULL, NULL);
> > > + show_textbox_ext(title, text, r, c, NULL, NULL, NULL, NULL);
> > > }
> > >
> > > static void show_helptext(const char *title, const char *text)
> > > @@ -381,35 +382,51 @@ static void show_help(struct menu *menu)
> > >
> > > struct search_data {
> > > struct list_head *head;
> > > - struct menu **targets;
> > > - int *keys;
> > > + struct menu *target;
> > > };
> > >
> > > -static void update_text(char *buf, size_t start, size_t end, void *_data)
> > > +static int next_key(int key)
> > > +{
> > > + key++;
> > > +
> > > + if (key > '9')
> > > + key = '1';
> > > +
> > > + return key;
> > > +}
> > > +
> > > +static int handle_search_keys(int key, int start, int end, void *_data)
> > > {
> > > struct search_data *data = _data;
> > > struct jump_key *pos;
> > > - int k = 0;
> > > +
> > > + if (key < '1' || key > '9')
> > > + return 0;
> > >
> > > list_for_each_entry(pos, data->head, entries) {
> > > - if (pos->offset >= start && pos->offset < end) {
> Sorry forgot to mention this, but start and end should be size_t.
> You get -Wsign-compare.
>
> > > - char header[4];
> > > + if (pos->offset >= start) {
> > > + if (pos->offset >= end)
> > > + break;
> > >
> > > - if (k < JUMP_NB) {
> > > - int key = '0' + (pos->index % JUMP_NB) + 1;
> > > -
> > > - sprintf(header, "(%c)", key);
> > > - data->keys[k] = key;
> > > - data->targets[k] = pos->target;
> > > - k++;
> > > - } else {
> > > - sprintf(header, " ");
> > > + if (key == '1' + (pos->index % JUMP_NB)) {
> > > + data->target = pos->target;
> > > + return 1;
> > > }
> > > -
> > > - memcpy(buf + pos->offset, header, sizeof(header) - 1);
> > > }
> > > }
> > > - data->keys[k] = 0;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int get_jump_key(void)
> > > +{
> > > + int cur_key;
> > > +
> > > + cur_key = jump_key;
> >
> > There should also be a check to see if jump_key is valid.
> > jump_key can be set to 0 and will have weird effects.
> >
> > > +
> > > + jump_key = next_key(cur_key);
> > > +
> > > + return cur_key;
> > > }
> > >
> > > static void search_conf(void)
> > > @@ -456,26 +473,23 @@ static void search_conf(void)
> > > sym_arr = sym_re_search(dialog_input);
> > > do {
> > > LIST_HEAD(head);
> > > - struct menu *targets[JUMP_NB];
> > > - int keys[JUMP_NB + 1], i;
> > > struct search_data data = {
> > > .head = &head,
> >
> > .target = NULL,
> > We check if target is null later on, we can make it more explicit
> >
> > > - .targets = targets,
> > > - .keys = keys,
> > > };
> > > struct jump_key *pos, *tmp;
> > >
> > > + jump_key = '1';
> > > res = get_relations_str(sym_arr, &head);
> > > set_subtitle();
> > > dres = show_textbox_ext("Search Results", str_get(&res), 0, 0,
> > > - keys, &vscroll, &hscroll, &update_text,
> > > - &data);
> > > + &vscroll, &hscroll,
> > > + handle_search_keys, &data);
> > > again = false;
> > > - for (i = 0; i < JUMP_NB && keys[i]; i++)
> > > - if (dres == keys[i]) {
> > > - conf(targets[i]->parent, targets[i]);
> > > - again = true;
> > > - }
> > > + if (dres >= '1' && dres <= '9') {
> > > + assert(data.target != NULL);
> > > + conf(data.target->parent, data.target);
> > > + again = true;
> > > + }
> > > str_free(&res);
> > > list_for_each_entry_safe(pos, tmp, &head, entries)
> > > free(pos);
> >
> > while here the formatting on the above line is one extra tab indented.
> >
> >
> > > diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> > > index b90fff833588..5578b8bc8a23 100644
> > > --- a/scripts/kconfig/menu.c
> > > +++ b/scripts/kconfig/menu.c
> > > @@ -701,6 +701,11 @@ static void get_dep_str(struct gstr *r, struct expr *expr, const char *prefix)
> > > }
> > > }
> > >
> > > > > +int __attribute__((weak)) get_jump_key(void)
> > > > This seems like a non-optimal solution, otherwise fine.
> > > Do you have a better idea?
> >
> > I do not.
> >
> > > +{
> > > + return -1;
> > > +}
> > > +
> > > static void get_prompt_str(struct gstr *r, struct property *prop,
> > > struct list_head *head)
> > > {
> > > @@ -743,11 +748,22 @@ static void get_prompt_str(struct gstr *r, struct property *prop,
> > > }
> > >
> > > str_printf(r, " Location:\n");
> > > - for (j = 4; --i >= 0; j += 2) {
> > > + for (j = 0; --i >= 0; j++) {
> > > + int jk = -1;
> > > + int indent = 2 * j + 4;
> > > +
> > > menu = submenu[i];
> > > - if (jump && menu == location)
> > > + if (jump && menu == location) {
> > > jump->offset = strlen(r->s);
> > > - str_printf(r, "%*c-> %s", j, ' ', menu_get_prompt(menu));
> > > + jk = get_jump_key();
> > > + }
> > > +
> > > + if (jk >= 0) {
> > > + str_printf(r, "(%c)", jk);
> > > + indent -= 3;
> > > + }
> > > +
> > > + str_printf(r, "%*c-> %s", indent, ' ', menu_get_prompt(menu));
> > > if (menu->sym) {
> > > str_printf(r, " (%s [=%s])", menu->sym->name ?
> > > menu->sym->name : "<choice>",
> > > --
> > > 2.39.2
> > >

Hi Masahiro Yamada,

Any updates?

Thanks,
Jesse Taube