Re: [PATCH] let dump and layout commands accept INDEX argument

[Thorsten Wißmann <re06huxa _at_ cip _dot_ cs _dot_ fau _dot_ de>]

On Wed, Nov 14, 2012 at 11:07:57AM +0800, Cheer Xiao wrote:
> 2012/11/14 Cheer Xiao <xiaqqaix _at_ gmail _dot_ com>:
> - The argument is optional and defaults to the root frame
> - lookup_frame() introduced, frame_insert_window_at_index() gone
> - new special frame index "@" which always refers to the currently focused
>   frame

OK, it took me some time to understand why you need an extra "@". But
now I think it's OK.

> - print_tag_tree() basically renamed to print_frame_tree(); an old
>   unimplemented print_frame_tree() declaration removed
> - herbstluftwm.txt: since the use of frame index is no longer restricted to
>   the "index" rule consequence, the description of frame index is a separate
>   section

The documentation looks good.

> diff --git a/src/layout.c b/src/layout.c
> index 4266637..4548776 100644
> +HSFrame* lookup_frame(HSFrame* root, char *index) {
> +    if (!strcmp(index, "@")) {
> +        return g_cur_frame;
> +    }

That's wrong. g_cur_frame points to the currently focused frame. But you
have to return the focused frame of the client's tag (and the client
isn't necessarily on the current tag).

> +    char* p;
> +    HSFrame* frame = root;
> +    for (p = index; *p; p++) {
> [...]
>          }
> -        frame_insert_window_at_index(frame, window, index + 1);
>      }
> +    return frame;
>  }

Why did you remove the tail-recursion? I don't think there is any
advantage of a for loop in that case. I would write something like this:
(untested!)

HSFrame* lookup_frame(HSFrame* root, char* index) {
    if (!index || index[0] == '\0') return root;
    if (root->type == TYPE_CLIENTS) return root;
    HSLayout* layout = &frame->content.layout;
    char* new_index = index + 1;
    HSFrame* frame;
    switch (index[0]) {
        case '0': frame = layout->a; break;
        case '1': frame = layout->b; break;
        /* opposite selection */
        case '/': frame = (layout->selection == 0)
                            ?  layout->b
                            :  layout->a;
                  break;
        /* else just follow selection */
        case '@': new_index = index;
        case '.':
        default:  frame = (layout->selection == 0)
                            ?  layout->a
                            : layout->b; break;
    }
    return lookup_frame(frame, new_index);
}

> -    if (argc >= 2) {
> +    // an empty argv[1] means current focused tag
> +    if (argc >= 2 && argv[1][0] != '\0') {
>          tag = find_tag(argv[1]);

Please also add that comment to the man page and I'll be happy.

Regards,
Thorsten