Re: [PATCH] Rule Names

[Thorsten Wißmann <edu _at_ thorsten _minus_ wissmann _dot_ de>]

Hi Tylo,

On Mon, Jan 21, 2013 at 03:05:32PM -0800, Tyler Thomas Hart wrote:
> This patchset is finally cleaned up and should be
> ready to go.

Thanks! Good work! Finally I applied it. It took much time, sorry for
that! I only changed one tiny thing. I also made some changes in extra
commits leaving yours unchanged. (See below)

The naming is a bit strange. Normally, ID (identifiers) are unique. But
it seems useful to allow multiple rules to have the same "identifier";
e.g. one script called myscript.sh creates some fancy rules all tagged
with id=myscript and can clean things up with: unrule myscript

But we can't use tag=myscript or name=myscript, so what to do? Find
another term? Leave "id" although it is not unique?

> Subject: [PATCH 1/4] Add initial support for rule ids
> 
>  void rules_destroy() {
>      g_queue_foreach(&g_rules, (GFunc)rule_destroy, NULL);
>      g_queue_clear(&g_rules);
> +    g_rule_id_index = 0;
>  }

The function rule_destroy() only is called on hlwm shutdown, so there is
no need to reset the counter. I removed it, just to prevent future
readers from getting confused here. (I changed your patch)

>  HSRule* rule_create() {
>      HSRule* rule = g_new0(HSRule, 1);
>      rule->once = false;
>      rule->birth_time = get_monotonic_timestamp();
> +    // Add name. Defaults to index number.
> +    GString* name = g_string_sized_new(20);
> +    g_string_printf(name, "%llu", g_rule_id_index++);
> +    rule->id = g_strdup(name->str);
> +    g_string_free(name, true);
>      return rule;
>  }

I never used it before, but here it seems to be useful to call
g_string_free(name, false) instead of g_strdup(). I committed this
later.

> +    if (printid) {
> +       g_string_append_printf(output, "ruleid\t%s\n", rule->id);
> +    }

Why that prefix? It would be much more scripting-friendly if just the id
is printed. (Also in the later commit)

> >From 0ef6f2e75b311a903fd59c2a5c221580116f9746 Mon Sep 17 00:00:00 2001
> Subject: [PATCH 2/4] Allow unrule command to remove by id
> +// Compares the id of two rules.
> +static gint rule_compare_id(const HSRule* a, const HSRule* b) {
> +    return (!strcmp(a->id, b->id)) ? 0 : -1;
> +}

I committed this to: return strcmp(a->id, b->id);

> Subject: [PATCH 3/4] Add command list_rules

Looks good. (I didn't know also can use %i instead of %d for printf)

> Subject: [PATCH 4/4] Add command completion for rule ids

It's OK, but you can have multiple entries in the completion table, so
there is no need to complete the '--all' and '-F' in the function. To
avoid the global value in rules.h, I moved the completion function to
rules.c. (all in extra commits, not changing yours)

Regards,
Thorsten