[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] error messages
- To: herbstluftwm _minus_ devel _at_ lists _dot_ sourceforge _dot_ net
- Subject: Re: [PATCH] error messages
- From: Florian Bruhin <me _at_ the _minus_ compiler _dot_ org>
- Date: Wed, 21 Nov 2012 08:09:10 +0100
Sorry for the late answer!
* Thorsten Wißmann <re06huxa _at_ cip _dot_ cs _dot_ fau _dot_ de> [2012-11-18 04:15:54 +0100]:
> thanks for your effort you put into that large patch set. I merged most
> of them (M-M-M-Monster-Merge), with some modifications or additions as
> listed below.
Yay! :)
> > -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?
> > int getenv_command(int argc, char** argv, GString* output) {
> > @@ -363,28 +381,34 @@ int getenv_command(int argc, char** argv, GString* output) {
> > }
> > char* envvar = getenv(argv[1]);
> > if (envvar == NULL) {
> > + g_string_append_printf(output,
> > + "Environment variable \"%s\" is not set!\n", argv[1]);
> > return HERBST_ENV_UNSET;
> > }
> > g_string_append_printf(output, "%s\n", envvar);
> > return 0;
> > }
>
> An unset environment variable in that context is not an error, I want
> the behaviour of "herbstclient get foo" is similar to "echo $foo". And
> one still can check if the variable was set or not by checking the error
> message. That's also why the exit status is HERBST_ENV_UNSET and not
> just "invalid ...". An intended usage is calling that in if-clauses (if
> ! hc get SSH_AGENT_PID ; then ...). So I removed that from your patch!
You're absolutely right of course.
> > -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.
> > diff --git a/src/rules.c b/src/rules.c
> > else {
> > - fprintf(stderr, "rule: unknown argument \"%s\"\n", *argv);
> > + // need to hardcode "rule:" here because args are shifted
> > + g_string_append_printf(output,
> > + "rule: unknown argument \"%s\"\n", *argv);
> > return HERBST_INVALID_ARGUMENT;
>
> Or remember it as char* cmdname = argv[0]; before the first SHIFT
Makes sense because I did that everywhere else. Will change that.
> > + if (output->len > 0) { // if there was output
> > + g_string_prepend(output, "rule: ");
> > + }
> > g_queue_push_tail(&g_rules, rule);
> > return 0;
>
> I removed that hunk because: 1. I can't see how this can be triggered
> (because you return on any error before). 2. I currently want the
> rule-command to output anything if it returns 0, because I plan to add
> IDs for rules that are printed on creation if the "printid"-flag is
> passed. (The ID then can be used to delete that single rule).
Yep, as you can see in the later commits there was a bit of a mess
with the rule name commits.
> > 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?
> > Subject: [PATCH 08/25] Print error when frame neighbours are missing
>
> That's OK, as there is no error message visible if the user calls that
> command with a key binding. It should always be OK to press (even
> useless) key-combinations on any layout tree.
I fully agree, and that was my thought as well.
> > Subject: [PATCH 10/25] Make arguments mandatory in jumpto/bring
> > int raise_command(int argc, char** argv) {
> > + if (argc < 2) {
> > + return HERBST_NEED_MORE_ARGS;
> > + }
> > Window win;
> > HSClient* client = NULL;
> > - win = string_to_client((argc > 1) ? argv[1] : "", &client);
> > + win = string_to_client(argv[1], &client);
> > if (client) {
> > client_raise(client);
> > } else {
>
> Actually we said, that it makes sense for raise. So i removed that hunk
> and corrected it in the documentation.
dafuq? I'm absolutely sure I amended the commit (I *did* change the
commit message) and removed the raise hunk. Maybe I accidentally did
commit -a or so afterwards...
> > 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.
> > 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?
> > Subject: [PATCH 19/25] Avoid shifting in monitor commands
>
> Why? IMO shifting the command name away make indices much more readable.
> So I'm skipping this patch.
Meh, you're right. Annoyed me because argv[0] was gone then, and I
probably overreacted a bit :P
> > Subject: [PATCH 21/25] Handle argv-shifting correctly in settings
> > int settings_cycle_value(int argc, char** argv, GString* output) {
> > + char* cmd_name = argv[0];
> > + char* setting_name = argv[1]; // save this before shifting
> > if (argc < 3) {
> > return HERBST_NEED_MORE_ARGS;
> > }
>
> I moved the the assignments after the argc-check.
Ohshit. A shame I didn't catch this one. Thanks!
> > 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)
Florian
--
() ascii ribbon campaign - stop html mail www.asciiribbon.org
/\ www.the-compiler.org | I love long mails http://email.is-not-s.ms/
It is a poor judge who cannot award a prize.