Re: [PATCH] Monitor names

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

Hi Florian,

On Mon, Dec 10, 2012 at 06:48:22AM +0100, Florian Bruhin wrote:
> So yeah, I implemented monitor names :)
> As previously mentioned I'll probably correct the small changes you
> proposed in the error messages patchset soon. If you have more time
> than me currently, feel free to do it yourself ;)

Nice patchset. I corrected the following things:

> Subject: [PATCH 04/10] Add string_to_monitor/monitor_index functions
> +HSMonitor* string_to_monitor(char* string) {
> +    if (string[0] == '\0') {
> +        return NULL;
> +    }
> +    if (isdigit(string[0])) {
> +        // monitor index
> +        int idx = atoi(string);
> +        if (idx < 0 || idx >= g_monitors->len) {
> +            return NULL;
> +        }
> +        return monitor_with_index(idx);
> +    } else {
> +        // monitor string
> +        return find_monitor_by_name(string);
> +    }
> +}

I shortened that to:

+HSMonitor* string_to_monitor(char* string) {
+    int idx = string_to_monitor_index(string);
+    return monitor_with_index(idx);
+}

This also removes redundancy. I'll probably also disallow monitor names
to start with + or - so that we can treat those as relative indices to
the index of the currently focused monitor.

> Subject: [PATCH 06/10] Add monitor names to all commands using monitors
> --- a/src/main.c
> +++ b/src/main.c
> @@ -261,13 +261,19 @@ int load_command(int argc, char** argv, GString* output) {
>  }
>  
>  int print_tag_status_command(int argc, char** argv, GString* output) {
> +    HSMonitor* monitor;
>      int monitor_index = g_cur_monitor;
>      if (argc >= 2) {
> -        monitor_index = atoi(argv[1]);
> +        monitor = string_to_monitor(argv[1]);
> +    } else {
> +        monitor = monitor_with_index(g_cur_monitor);

I changed that to get_current_monitor();

> Subject: [PATCH 07/10] Add monitor argument to list_padding

Here I added an extra explanation to the commit message â more or less
what you already stated in the MIGRATION file.

> Subject: [PATCH 08/10] Add new command name_monitor

Analogous to "rename" which renames tags, "rename_monitor" IMO is a more
appropriate name. You gave your OK in the irc so i changed it.

> Subject: [PATCH 09/10] Implement completion for monitor names

Yay! I love command completion.

> +    { "name_monitor",   3,  no_completion },

You already added that the commit before, so I removed that line. You
also made the g_monitors array public. It should stay
monitor.c-internal, there are monitor_with_index() and monitor_count()
functions to access the content. The reason for this is, that you have
to cast the type manually each time you access g_monitors directly,
which is error-prone.

> Subject: [PATCH 10/10] Add monitor names to shift_to_monitor

That's also OK. And here, monitor names aren't accepted if they start
with - or +, so I'll commit this on top of your patchset that
string_to_monitor_index() already treats relative indices correctly.

Your changes are now in the public master branch.

Happy Holidays,
Regards,
Thorsten