Re: [cwm 1/1] Add a menu to search and call internal functions.
> > Why not
> >
> > for (iter = 0; iter < nitems(name_to_kbfunc); iter++) {
> > func = &name_to_kbfunc[iter]; /* if you want to use func */
> >
> > then you don't need the last NULL entry in the array or add code
> > to not break existing code.
>
> See: http://c-faq.com/decl/extarraysize.html
>
> The diff moves the declaration of name_to_kbfunc[] to calmwm.h and
> externs it for use in kbfunc.c, but it's still defined in conf.c.
>
Yeah, I see that now, though the use of nitems is used throughout
the code already and deviating from that and making that particular
array a "special case" seems kind of messy and somewhat confusing.
Re: [cwm 1/1] Add a menu to search and call internal functions.
Hey, dude-
> I too would prefer to use nitems, to be consistent with the rest of the
> code. Also reduces the number of gratuitous changes, and of course the
> size of the diff.
I chose the guard element approach because it leads to the smallest diff,
but I can move the definition of name_to_kbfunc[] from conf.c to a header
if you prefer.
> > > + if ((mi = menu_filter(sc, &menuq, "function", NULL, 0,
> > > + search_match_text, NULL)) != NULL) {
> > > + func = (struct func *)mi->ctx;
> > > + (*func->handler)(cc, &func->argument);
>
> Likewise, as mentioned, consistency with the rest is best.
I wrote the above specifically to be consistent with other parts of cwm;
see: xevents.c:261 and xevents.c:314.
Thanks for the feedback!
Best,
Kent
Re: [cwm 1/1] Add a menu to search and call internal functions.
Hey, dude-
> Can you please provide a unified cvs diff?
The first patches I sent last week were cvs diffs, but I saw quilt
was recently added to ports to I switched to using it to manage all
of the patches (I have a few other things I'm working on, too, which
I haven't sent out for review yet).
Does anyone know if quilt can delegate diffing to cvs? I'll
investigate some more and see if I can get quilt to do this
automatically.
> Why do you insist on adding this? Why not just use nitems?
...
> See? Now you have to add more code because you broke something.
...
> Why not
>
> for (iter = 0; iter < nitems(name_to_kbfunc); iter++) {
> func = &name_to_kbfunc[iter]; /* if you want to use func */
>
> then you don't need the last NULL entry in the array or add code
> to not break existing code.
See: http://c-faq.com/decl/extarraysize.html
The diff moves the declaration of name_to_kbfunc[] to calmwm.h and
externs it for use in kbfunc.c, but it's still defined in conf.c.
> > + if ((mi = menu_filter(sc, &menuq, "function", NULL, 0,
> > + search_match_text, NULL)) != NULL) {
> > + func = (struct func *)mi->ctx;
> > + (*func->handler)(cc, &func->argument);
>
> "func->handler(..." is fine but some people prefer the pre-ANSI
> C style, "(*func->handler)(..." ... Just a tiny nit. YMMV.
I don't have a preference, I just followed the style used elsewhere
in cwm.
Thanks for the feedback!
Best,
Kent
Re: [cwm 1/1] Add a menu to search and call internal functions.
I haven't caught-up, nor reviewed anything else yet, but comments
inline:
On Tue 2012.11.06 at 16:11 +0100, Thomas Pfaff wrote:
> Can you please provide a unified cvs diff?
'cvs diff -uNp' is best...
> [...]
> > for (iter = 0; iter < nitems(name_to_kbfunc); iter++) {
> > - if (strcmp(name_to_kbfunc[iter].tag, binding) != 0)
> > + if (name_to_kbfunc[iter].tag == NULL ||
> > + strcmp(name_to_kbfunc[iter].tag, binding) != 0)
> > continue;
I too would prefer to use nitems, to be consistent with the rest of the
code. Also reduces the number of gratuitous changes, and of course the
size of the diff.
> > + if ((mi = menu_filter(sc, &menuq, "function", NULL, 0,
> > + search_match_text, NULL)) != NULL) {
> > + func = (struct func *)mi->ctx;
> > + (*func->handler)(cc, &func->argument);
Likewise, as mentioned, consistency with the rest is best.
Re: [cwm 1/1] Add a menu to search and call internal functions.
Can you please provide a unified cvs diff?
I've not tried this patch but I have some comments (see below).
> Useful for those times you want to use an unbound function, but even
> when the function is bound to something you haven't memorized yet it
> can be faster than lookup up the keybinding in the manual. Bound to
> CM-/ by default.
>
[...]
>
> +struct func name_to_kbfunc[] = {
> { "lower", kbfunc_client_lower, KBFLAG_NEEDCLIENT, {0} },
> { "raise", kbfunc_client_raise, KBFLAG_NEEDCLIENT, {0} },
> { "search", kbfunc_client_search, 0, {0} },
> + { "funcsearch", kbfunc_func_search, KBFLAG_NEEDCLIENT, {0} },
> { "menusearch", kbfunc_menu_search, 0, {0} },
> { "hide", kbfunc_client_hide, KBFLAG_NEEDCLIENT, {0} },
> { "cycle", kbfunc_client_cycle, 0, {.i = CWM_CYCLE} },
> @@ -399,6 +396,7 @@
> {.i = (CWM_LEFT|CWM_PTRMOVE|CWM_BIGMOVE)} },
> { "bigptrmoveright", kbfunc_moveresize, 0,
> {.i = (CWM_RIGHT|CWM_PTRMOVE|CWM_BIGMOVE)} },
> + { NULL, NULL, 0, {0} }
Why do you insist on adding this? Why not just use nitems?
> @@ -488,7 +486,8 @@
> return;
>
> for (iter = 0; iter < nitems(name_to_kbfunc); iter++) {
> - if (strcmp(name_to_kbfunc[iter].tag, binding) != 0)
> + if (name_to_kbfunc[iter].tag == NULL ||
> + strcmp(name_to_kbfunc[iter].tag, binding) != 0)
> continue;
See? Now you have to add more code because you broke something.
> Index: cwm/kbfunc.c
> ===
> --- cwm.orig/kbfunc.c 2012-11-05 10:25:37.577424801 -0600
> +++ cwm/kbfunc.c 2012-11-05 14:17:27.164487245 -0600
> @@ -174,6 +174,35 @@
> }
>
> void
> +kbfunc_func_search(struct client_ctx *cc, union arg *arg)
> +{
> + struct screen_ctx *sc = cc->sc;
> + struct func *func;
> + struct menu *mi;
> + struct menu_qmenuq;
> +
> + TAILQ_INIT(&menuq);
> +
> + for (func = name_to_kbfunc; func->tag != NULL; func++) {
Why not
for (iter = 0; iter < nitems(name_to_kbfunc); iter++) {
func = &name_to_kbfunc[iter]; /* if you want to use func */
then you don't need the last NULL entry in the array or add code
to not break existing code.
> + if ((mi = menu_filter(sc, &menuq, "function", NULL, 0,
> + search_match_text, NULL)) != NULL) {
> + func = (struct func *)mi->ctx;
> + (*func->handler)(cc, &func->argument);
"func->handler(..." is fine but some people prefer the pre-ANSI
C style, "(*func->handler)(..." ... Just a tiny nit. YMMV.
[cwm 1/1] Add a menu to search and call internal functions.
Useful for those times you want to use an unbound function, but even
when the function is bound to something you haven't memorized yet it
can be faster than lookup up the keybinding in the manual. Bound to
CM-/ by default.
Index: cwm/calmwm.h
===
--- cwm.orig/calmwm.h 2012-11-05 13:50:58.468179916 -0600
+++ cwm/calmwm.h2012-11-05 14:17:27.164487245 -0600
@@ -225,6 +225,13 @@
};
TAILQ_HEAD(screen_ctx_q, screen_ctx);
+struct func {
+ char*tag;
+ void (*handler)(struct client_ctx *, union arg *);
+ int flags;
+ union argargument;
+};
+
struct keybinding {
TAILQ_ENTRY(keybinding) entry;
void(*callback)(struct client_ctx *, union arg *);
@@ -392,6 +399,7 @@
union arg *);
voidkbfunc_cmdexec(struct client_ctx *, union arg *);
voidkbfunc_exec(struct client_ctx *, union arg *);
+voidkbfunc_func_search(struct client_ctx *, union arg *);
voidkbfunc_lock(struct client_ctx *, union arg *);
voidkbfunc_menu_search(struct client_ctx *, union arg *);
voidkbfunc_moveresize(struct client_ctx *, union arg *);
@@ -498,6 +506,7 @@
extern struct screen_ctx_q Screenq;
extern struct client_ctx_q Clientq;
extern struct conf Conf;
+extern struct func name_to_kbfunc[];
extern int HasXinerama, HasRandr, Randr_ev;
Index: cwm/conf.c
===
--- cwm.orig/conf.c 2012-11-05 10:25:37.577424801 -0600
+++ cwm/conf.c 2012-11-05 14:17:27.164487245 -0600
@@ -99,6 +99,7 @@
{ "M-Up", "raise" },
{ "M-slash","search" },
{ "C-slash","menusearch" },
+ { "CM-slash", "funcsearch" },
{ "M-Tab", "cycle" },
{ "MS-Tab", "rcycle" },
{ "CM-n", "label" },
@@ -284,15 +285,11 @@
cc->flags |= ignore ? CLIENT_IGNORE : 0;
}
-static struct {
- char*tag;
- void (*handler)(struct client_ctx *, union arg *);
- int flags;
- union argargument;
-} name_to_kbfunc[] = {
+struct func name_to_kbfunc[] = {
{ "lower", kbfunc_client_lower, KBFLAG_NEEDCLIENT, {0} },
{ "raise", kbfunc_client_raise, KBFLAG_NEEDCLIENT, {0} },
{ "search", kbfunc_client_search, 0, {0} },
+ { "funcsearch", kbfunc_func_search, KBFLAG_NEEDCLIENT, {0} },
{ "menusearch", kbfunc_menu_search, 0, {0} },
{ "hide", kbfunc_client_hide, KBFLAG_NEEDCLIENT, {0} },
{ "cycle", kbfunc_client_cycle, 0, {.i = CWM_CYCLE} },
@@ -399,6 +396,7 @@
{.i = (CWM_LEFT|CWM_PTRMOVE|CWM_BIGMOVE)} },
{ "bigptrmoveright", kbfunc_moveresize, 0,
{.i = (CWM_RIGHT|CWM_PTRMOVE|CWM_BIGMOVE)} },
+ { NULL, NULL, 0, {0} }
};
/*
@@ -488,7 +486,8 @@
return;
for (iter = 0; iter < nitems(name_to_kbfunc); iter++) {
- if (strcmp(name_to_kbfunc[iter].tag, binding) != 0)
+ if (name_to_kbfunc[iter].tag == NULL ||
+ strcmp(name_to_kbfunc[iter].tag, binding) != 0)
continue;
current_binding->callback = name_to_kbfunc[iter].handler;
Index: cwm/kbfunc.c
===
--- cwm.orig/kbfunc.c 2012-11-05 10:25:37.577424801 -0600
+++ cwm/kbfunc.c2012-11-05 14:17:27.164487245 -0600
@@ -174,6 +174,35 @@
}
void
+kbfunc_func_search(struct client_ctx *cc, union arg *arg)
+{
+ struct screen_ctx *sc = cc->sc;
+ struct func *func;
+ struct menu *mi;
+ struct menu_qmenuq;
+
+ TAILQ_INIT(&menuq);
+
+ for (func = name_to_kbfunc; func->tag != NULL; func++) {
+ mi = xcalloc(1, sizeof(*mi));
+ (void)strlcpy(mi->text, func->tag, sizeof(mi->text));
+ mi->ctx = func;
+ TAILQ_INSERT_TAIL(&menuq, mi, entry);
+ }
+
+ if ((mi = menu_filter(sc, &menuq, "function", NULL, 0,
+ search_match_text, NULL)) != NULL) {
+ func = (struct func *)mi->ctx;
+ (*func->handler)(cc, &func->argument);
+ }
+
+ while ((mi = TAILQ_FIRST(&menuq)) != NULL) {
+ TAILQ_REMOVE(&menuq, mi, entry);
+ xfree(mi);
+ }
+}
+
+void
kbfunc_menu_search(struct client_ctx *cc, union arg *arg)
{
struct screen_ctx *sc = cc->sc;
Index: cwm/cwm.1
===
--- cwm.orig/cwm.1 2012-10-31 17:12:11.71058 -0500
+++ cwm/cwm.1 2012-11-05 14:21:15.746828265 -0600
@@ -70,6 +70,8 @@
Search f
