Re: ratpoison, patches, and the future
[email protected] (Jérémie Courrèges-Anglas) Wed, 31 Dec 2014 11:32:20 +0100
| Newsgroups | gmane.comp.window-managers.ratpoison.devel |
|---|---|
| Message-ID | <[email protected]> |
Jeff Abrahamson <[email protected]> writes: [...] > Jérémie sent me some code feedback in private. Most of it was a bit > ordinary stuff that needn't be repeated. Worth repeating here, however: [...] > - He's not so keen on my refactoring of cmd_select() and > set_active_window_body(), suggesting they don't bring real improvement. I > disagree. Both functions were overly long to my eye and harder to > understand for it. Let's take a look at cmd_select() and what refactoring could *actually* be useful. > cmd_select (int interactive UNUSED, struct cmdarg **args) > { > cmdret *ret = NULL; > char *str; > int n; > > /* FIXME: This is manually done because of the kinds of things > select accepts. */ > if (args[0] == NULL) > str = get_input (MESSAGE_PROMPT_SWITCH_TO_WINDOW, hist_SELECT, > window_completions); > else > str = xstrdup (ARG_STRING(0)); > > /* User aborted. */ > if (str == NULL) > return cmdret_new (RET_FAILURE, NULL); > > /* Only search if the string contains something to search for. */ > if (strlen (str) > 0) Here we add an indentation level when we could just return. > { > if (strlen (str) == 1 && str[0] == '-') Here we could use strcmp(3). But just moving this block of code into yet another function (badly named, it's not a command) won't make the rest of the code any easier to read. It is already readable. [...] > } > else > /* Silently fail, since the user didn't provide a window spec */ > ret = cmdret_new (RET_SUCCESS, NULL); > > free (str); > > return ret; > } > (A quick git annotate on window.c even suggests that > Jérémie may be the author of the FIXME on set_active_window_body(). ;-) Too quick. e56b2ca is a revert. I un-did a similar refactoring that was actually incorrect for three reasons: two bugs introduced, and a FIXME comment removed, replaced by a wrong comment. Pseudo refactoring didn't magically help the author of the commit. "Refactor duplicate branches of if() into a single block with leading ?:." does makes sense, I had a similar commit in one of my local branches. "Refactor a bit of set_active_window_body() into a helper function." is the same mechanical change as the previous, incorrect one I've just discussed above, except that, while it looks right from a functionality PoV: - you did not bother moving / removing the FIXME comment while it does not make sense anymore where it is - it clutters the code with pointers to pointers instead of simple assignements - it does not address the first issue: the code is looking bad *right now*, splitting it into smaller chunks before cleaning it will only help us lose sight of the big picture. I pushed the first change, here's a wip proposal regarding further cleanup. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE _______________________________________________ Ratpoison-devel mailing list [email protected] https://lists.nongnu.org/mailman/listinfo/ratpoison-devel
0001-Third-try-at-cleaning-set_active_window_body.patch
(text/x-patch, 3.4 KB)
From d3e80615151073b4568cf70aaaeb4b50cc43c1a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Courr=C3=A8ges-Anglas?= <[email protected]> Date: Wed, 31 Dec 2014 10:15:06 +0100 Subject: [PATCH] Third try at cleaning set_active_window_body Delete the weird logic and the nested loops. Use a true helper function, don't just move code around. --- src/window.c | 84 ++++++++++++++++++++++++++++++------------------------------ 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/src/window.c b/src/window.c index d398b75..7d31a87 100644 --- a/src/window.c +++ b/src/window.c @@ -452,8 +452,41 @@ void set_active_window_force (rp_window *win) set_active_window_body(win, 1); } -/* FIXME: This function is probably a mess. I can't remember a time - when I didn't think this. It probably needs to be fixed up. */ +static rp_frame * +find_frame_non_dedicated(rp_screen *current_screen, rp_frame *current_frame) +{ + rp_frame *cur; + + /* Try the only / current screen... */ + for (cur = list_next_entry (current_frame, ¤t_screen->frames, node); + cur != current_frame; + cur = list_next_entry (cur, ¤t_screen->frames, node)) + { + if (!cur->dedicated) + return cur; + } + + /* If we have Xinerama, we can check *all* screens... */ + if (rp_have_xinerama) + { + int i; + + for (i = 0; i < num_screens; i++) + { + if (current_screen == &screens[i]) + continue; + + list_for_each_entry (cur, &screens[i].frames, node) + { + if (!cur->dedicated) + return cur; + } + } + } + + return NULL; +} + void set_active_window_body (rp_window *win, int force) { @@ -485,48 +518,15 @@ set_active_window_body (rp_window *win, int force) if (frame->dedicated && !force) { - /* Try to find a non-dedicated frame. */ - rp_frame *cur; - rp_screen *scr; - int done; - - scr = (rp_have_xinerama)?&screens[rp_current_screen]:win->scr; - done = 0; - - /* Try the only / current screen... */ - for (cur = list_next_entry (frame, &scr->frames, node); - cur != frame && !done; - cur = list_next_entry (cur, &scr->frames, node)) - { - if (!cur->dedicated) - { - set_active_frame (cur, 0); - last_frame = frame; - frame = cur; - done = 1; - } - } + /* Try to find a non-dedicated frame. */ + rp_frame *non_dedicated; - /* If we have Xinerama, we can check *all* screens... */ - if (rp_have_xinerama && !done) + non_dedicated = find_frame_non_dedicated (screen, frame); + if (non_dedicated != NULL) { - int i; - - for (i=0; i<num_screens && !done; i++) - { - if (scr == &screens[i]) continue; - list_for_each_entry (cur,&screens[i].frames,node) - { - if (!cur->dedicated) - { - set_active_frame (cur, 0); - last_frame = frame; - frame = cur; - done = 1; /* Break outer loop. */ - break; /* Break inner loop. */ - } - } - } + last_frame = frame; + frame = non_dedicated; + set_active_frame (frame, 0); } } -- 2.2.0