From b02e01822564e5bee3c31082e978419edea6a37c Mon Sep 17 00:00:00 2001 From: Michael Meyer Date: Fri, 7 Oct 2022 12:39:18 -0400 Subject: [PATCH 1/2] Remove explicit 'none' opt from autounlock handler The autounlock handler included an explicit 'none' option, a choice that gave it a different UX from similar existing compound option handlers (e.g. paranoid_confirm or pickup_types), which set 'none' simply by deselecting all options. It didn't make the menu any easier to use (at least in my experience), since in order to go from some combination of options to 'none', you'd have to deselect everything anyway (which on its own was enough to set 'none', so there was no reason to explicitly select it after doing so). Make the autounlock handler work like other compound option handlers, such that deselecting all options is the way to set 'none', and there is no explicit 'none' option included in the list. --- src/options.c | 45 +++++++++++++++------------------------------ 1 file changed, 15 insertions(+), 30 deletions(-) diff --git a/src/options.c b/src/options.c index 435586e0e..83895b8f1 100644 --- a/src/options.c +++ b/src/options.c @@ -193,7 +193,6 @@ static NEARDATA const char *msgwind[][3] = { /* 'msg_window' settings */ #endif /* autounlock settings */ static NEARDATA const char *unlocktypes[][2] = { - { "none", "" }, { "untrap", "(might fail)" }, { "apply-key", "" }, { "kick", "(doors only)" }, @@ -782,8 +781,10 @@ optfn_autounlock( op = trimspaces(op); /* might have trailing space after * plus sign removal */ } - for (i = 0; i < SIZE(unlocktypes); ++i) - if (!strncmpi(op, unlocktypes[i][0], Strlen(op)) + if (str_start_is("none", op, TRUE)) + negated = TRUE; + for (i = 0; i < SIZE(unlocktypes); ++i) { + if (str_start_is(unlocktypes[i][0], op, TRUE) /* fuzzymatch() doesn't match leading substrings but this allows "apply_key" and "applykey" to match "apply-key"; "apply key" too if part of foo+bar */ @@ -810,6 +811,7 @@ optfn_autounlock( return optn_silenterr; } } + } op = nxt; } if (negated && newflags != 0) { @@ -832,13 +834,13 @@ optfn_autounlock( *opts = '\0'; if (flags.autounlock & AUTOUNLOCK_UNTRAP) - Sprintf(eos(opts), "%s%s", p, unlocktypes[1][0]), p = plus; + Sprintf(eos(opts), "%s%s", p, unlocktypes[0][0]), p = plus; if (flags.autounlock & AUTOUNLOCK_APPLY_KEY) - Sprintf(eos(opts), "%s%s", p, unlocktypes[2][0]), p = plus; + Sprintf(eos(opts), "%s%s", p, unlocktypes[1][0]), p = plus; if (flags.autounlock & AUTOUNLOCK_KICK) - Sprintf(eos(opts), "%s%s", p, unlocktypes[3][0]), p = plus; + Sprintf(eos(opts), "%s%s", p, unlocktypes[2][0]), p = plus; if (flags.autounlock & AUTOUNLOCK_FORCE) - Sprintf(eos(opts), "%s%s", p, unlocktypes[4][0]); /*no more p*/ + Sprintf(eos(opts), "%s%s", p, unlocktypes[3][0]); /*no more p*/ } return optn_ok; } @@ -4856,38 +4858,21 @@ handler_autounlock(int optidx) for (i = 0; i < SIZE(unlocktypes); ++i) { Sprintf(buf, "%-10.10s%c%.40s", unlocktypes[i][0], sep, unlocktypes[i][1]); - presel = !i ? !flags.autounlock : (flags.autounlock & (1 << (i - 1))); + presel = (flags.autounlock & (1 << i)); any.a_int = i + 1; add_menu(tmpwin, &nul_glyphinfo, &any, *unlocktypes[i][0], 0, ATR_NONE, clr, buf, - ((presel ? MENU_ITEMFLAGS_SELECTED : MENU_ITEMFLAGS_NONE) - | (!i ? MENU_ITEMFLAGS_SKIPINVERT : 0))); + (presel ? MENU_ITEMFLAGS_SELECTED : MENU_ITEMFLAGS_NONE)); } Sprintf(buf, "Select '%.20s' actions:", optname); end_menu(tmpwin, buf); n = select_menu(tmpwin, PICK_ANY, &window_pick); if (n > 0) { - int k; - boolean wasnone = !flags.autounlock; - unsigned newflags = 0, noflags = 0; + unsigned newflags = 0; - for (i = 0; i < n; ++i) { - k = window_pick[i].item.a_int - 1; - if (k) - newflags |= (1 << (k - 1)); - else - noflags = 1; - } - /* wasnone: 'none' is preselected; - !wasnone: don't force it to be unselected */ - if (newflags && noflags && !wasnone) { - config_error_add( - "Invalid value combination for \"%s\": 'none' with some", - optname); - res = optn_silenterr; - } else { - flags.autounlock = newflags; - } + for (i = 0; i < n; ++i) + newflags |= (1 << (window_pick[i].item.a_int - 1)); + flags.autounlock = newflags; free((genericptr_t) window_pick); } else if (n == 0) { /* nothing was picked but menu wasn't cancelled */ /* something that was preselected got unselected, leaving nothing; From 883f973f5641205bb4d6ae6cbdbfac0a2566e0c4 Mon Sep 17 00:00:00 2001 From: Michael Meyer Date: Fri, 7 Oct 2022 13:18:14 -0400 Subject: [PATCH 2/2] Fix: error handling for invalid autounlock value Because the existing error was the default case in a switch/case statement only reachable if the option matched one of the expected ones in the list, it wasn't actually reachable: something totally out of left-field wouldn't match one of the expected options so never hit the switch, and something that did match one of the expected options would by definition have a first character handled by one of the cases in the switch/case. Do it a slightly different way that should successfully raise an unexpected value error for 'OPTIONS=autounlock:foobar'. I didn't remove the default case entirely, because it could still catch an error if some new value is added to unlocktypes[] without a corresponding case being added to the switch statement. --- src/options.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/options.c b/src/options.c index 83895b8f1..30efd23e2 100644 --- a/src/options.c +++ b/src/options.c @@ -775,6 +775,7 @@ optfn_autounlock( newflags = 0; sep = index(op, '+') ? '+' : ' '; while (op) { + boolean matched = FALSE; op = trimspaces(op); /* might have leading space */ if ((nxt = index(op, sep)) != 0) { *nxt++ = '\0'; @@ -782,13 +783,14 @@ optfn_autounlock( * plus sign removal */ } if (str_start_is("none", op, TRUE)) - negated = TRUE; - for (i = 0; i < SIZE(unlocktypes); ++i) { + negated = TRUE, matched = TRUE; + for (i = 0; i < SIZE(unlocktypes) && !matched; ++i) { if (str_start_is(unlocktypes[i][0], op, TRUE) /* fuzzymatch() doesn't match leading substrings but this allows "apply_key" and "applykey" to match "apply-key"; "apply key" too if part of foo+bar */ || fuzzymatch(op, unlocktypes[i][0], " -_", TRUE)) { + matched = TRUE; switch (*op) { case 'n': negated = TRUE; @@ -806,12 +808,16 @@ optfn_autounlock( newflags |= AUTOUNLOCK_FORCE; break; default: - config_error_add("Invalid value for \"%s\": \"%s\"", - allopt[optidx].name, op); - return optn_silenterr; + matched = FALSE; + break; } } } + if (!matched) { + config_error_add("Invalid value for \"%s\": \"%s\"", + allopt[optidx].name, op); + return optn_silenterr; + } op = nxt; } if (negated && newflags != 0) {