Re: [PATCH] error messages

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

Hi Florian,

On Wed, Nov 21, 2012 at 08:09:10AM +0100, Florian Bruhin wrote:
> * Thorsten Wißmann <re06huxa _at_ cip _dot_ cs _dot_ fau _dot_ de> [2012-11-18 04:15:54 +0100]:
> > > -int jumpto_command(int argc, char** argv) {
> > > +int jumpto_command(int argc, char** argv, GString* output) {
> > >      HSClient* client = NULL;
> > >      string_to_client((argc > 1) ? argv[1] : "", &client);
> > >      if (client) {
> > >          focus_window(client->window, true, true);
> > > +        return 0;
> > > +    } else {
> > > +        g_string_append_printf(output,
> > > +            "%s: error: Could not find client", argv[0]);
> > > +        if (argc > 1) {
> > > +            g_string_append_printf(output, " \"%s\".\n", argv[1]);
> > > +        } else {
> > > +            g_string_append(output, ".\n");
> > > +        }
> > > +        return HERBST_INVALID_ARGUMENT;
> > >      }
> > > -    return 0;
> > >  }
> > 
> > That's not perfect yet, because now, the exit status for a single
> > "jumpto" depends on, if a client or an empty frame is focused. I don't
> > think there is any real use case for this, so we can discuss about a
> > clean long-term solution and merge this for now.
> 
> I guess I need to read what string_to_client() looks like again.
> Am I right when assuming I could simply do something like
> 
>     } else if (!client && argc > 1) {
> 
> so that only triggers with an actual argument and always returns 0
> if no explicit client is given?

Then there still would be a difference between jumpto and jumpto "". But
maybe it's OK to return an error code (or even useful in combination
with and/or). So this can be left like it currently is.

> > > -int detect_monitors_command(int argc, char **argv) {
> > > +int detect_monitors_command(int argc, char **argv, GString* output) {
> > >      MonitorDetection detect[] = {
> > >          detect_monitors_xinerama,
> > >          detect_monitors_simple,
> > > @@ -923,6 +947,10 @@ int detect_monitors_command(int argc, char **argv) {
> > >      // apply it
> > >      int ret = set_monitor_rects(monitors, count);
> > >      g_free(monitors);
> > > +    if (ret == HERBST_TAG_IN_USE && output != NULL) {
> > > +        g_string_append_printf(output,
> > > +            "%s: there are not enough free tags!\n", argv[0]);
> > > +    }
> > >      return ret;
> > >  }
> > 
> > Why do you check output here? You can use it unchecked, just like
> > anywhere else (or like argv).
> 
> As I quickly told you in IRC already: because detect_monitors_command
> gets called internally in main.c:730 (configurenotify) like this:
> 
>     detect_monitors_command(LENGTH(args), args, NULL);
> 
> Exactly the same thing again in settings_set_command at
> settings.c:123 which gets called in layout.c:101 in
> fetch_frame_colors.
> 
> Since I couldn't think of a better solution, I decided to simply check
> if the output-pointer is NULL in functions which get used internally,
> and call them like  foo(argc, argv, NULL);
> 
> If you'd remove that, it wouldn't be fatal, but you'll get a failed
> assert from glib:
> 
>     (process:4083): GLib-CRITICAL **: g_string_append: assertion
>     `string != NULL' failed
> 
> It seems like the checks are still in master, if you can think of any
> better solution let me know.

Yes, that's OK but I don't know a better solution.

> > > Subject: [PATCH 05/25] Return status code in key_remove_bind_with_keysym
> > 
> > If it's not a command, then the status code meaning isn't that
> > intuitive, so I prefer a bool here, so we know true is good and false is
> > bad. I just amended your commit. Which also affects the following
> > commit:
> 
> I agree, but I wasn't sure what you use yourself, because *I think*
> I've seen both in your code (in non-command functions). Good to know.
> Maybe things like this should be added to HACKING?

I added this and some other pointers to the HACKING file.

> > > Subject: [PATCH 12/25] Cleanup of error messages
> > > Subject: [PATCH 13/25] Add an error message if "raise" can't find client
> > 
> > That's fine. (After some manual patch fixing ...).
> 
> Huh, why manual patch fixing? Just because it didn't apply anymore?
> If I was you, I'd apply all patches in a separate branch, and then
> rebase that branch to change things. Then you can resolve conflicts
> easily.

I forgot to apply them all in one branch. The problem occurred first
after I already merged some patches... I'll do this in the future.

> > > Subject: [PATCH 18/25] prepend "load: " to msgs in load_command
> > > +    if (output->len > 0) {
> > > +        g_string_prepend(output, "load: ");
> > > +    }
> > 
> > That's very hacky and only works because the load command normally does
> > not output anything. But I currently neither have a better idea nor
> > plans for a possible output of the "load" command.
> 
> Well, I guess I could add "load: " by hand to every error message. Do
> you think that would be better?

Yes for the long-term I think it's better to add it each time.

> > > Subject: [PATCH 22/25] Add error message if tag can't be switched
> > > +            g_string_append_printf(output,
> > > +                "%s: Could not change tag (maybe monitor is locked?)\n", argv[0]);
> > > +        }
> > 
> > Here I also like the monitor_set_tag function to return a bool
> > indicating the success/failure. And don't ask the user those question
> > you (in the role hlwm) can answer! That definitely must be fixed, I
> > merged it anyway to simplify the remaining patches.
> 
> I agree. Will also fix that.
> 
> I'll send you a cleanup patch as soon as you tell me what to think
> (and I have some spare time)

Thanks for your effort! It does not hurry.

Thorsten