command: change cycle-value command behavior

Instead of using an internal counter to keep track of the value that was
set last, attempt to find the current value of the property/option in
the value list, and then set the next value in the list.

There are some potential problems. If a property refuses to accept a
specific value, the cycle-values command will fail, and start from the
same position again. It can't know that it's supposed to skip the next
value. The same can happen to properties which behave "strangely", such
as the "aspect" property, which will return the current aspect if you
write "-1" to it. As a consequence, cycle-values can appear to get
"stuck".

I still think the new behavior is what users expect more, and which is
generally more useful. We won't restore the ability to get the old
behavior, unless we decide to revert this commit entirely.

Fixes #5772, and hopefully other complaints.

1 files changed, 88 insertions, 73 deletions

diff --git a/player/command.c b/player/command.c
index fa0151c6b8..a21fc53abb 100644
--- a/player/command.c
+++ b/player/command.c
@@ -84,9 +84,6 @@ struct command_ctx {
     char **warned_deprecated;
     int num_warned_deprecated;
 
-    struct cycle_counter *cycle_counters;
-    int num_cycle_counters;
-
     struct overlay *overlays;
     int num_overlays;
     // One of these is in use by the OSD; the other one exists so that the
@@ -4671,47 +4668,6 @@ static void overlay_uninit(struct MPContext *mpctx)
         mp_image_unrefp(&cmd->overlay_osd[n].packed);
 }
 
-struct cycle_counter {
-    char **args;
-    int counter;
-};
-
-static bool stringlist_equals(char **l1, char **l2)
-{
-    assert(l1 && l2);
-    for (int i = 0; ; i++) {
-        if (!l1[i] && !l2[i])
-            return true;
-        if (!l1[i] || !l2[i])
-            return false;
-        if (strcmp(l1[i], l2[i]) != 0)
-            return false;
-    }
-}
-
-static char **stringlist_dup(void *talloc_ctx, char **list)
-{
-    int num = 0;
-    char **res = NULL;
-    for (int i = 0; list && list[i]; i++)
-        MP_TARRAY_APPEND(talloc_ctx, res, num, talloc_strdup(talloc_ctx, list[i]));
-    MP_TARRAY_APPEND(talloc_ctx, res, num, NULL);
-    return res;
-}
-
-static int *get_cmd_cycle_counter(struct MPContext *mpctx, char **args)
-{
-    struct command_ctx *cmd = mpctx->command_ctx;
-    for (int n = 0; n < cmd->num_cycle_counters; n++) {
-        struct cycle_counter *ctr = &cmd->cycle_counters[n];
-        if (stringlist_equals(ctr->args, args))
-            return &ctr->counter;
-    }
-    struct cycle_counter ctr = {stringlist_dup(cmd, args), -1};
-    MP_TARRAY_APPEND(cmd, cmd->cycle_counters, cmd->num_cycle_counters, ctr);
-    return &cmd->cycle_counters[cmd->num_cycle_counters - 1].counter;
-}
-
 static struct track *find_track_with_url(struct MPContext *mpctx, int type,
                                          const char *url)
 {
@@ -4752,8 +4708,8 @@ static bool check_property_scalable(char *property, struct MPContext *mpctx)
            prop.type == &m_option_type_aspect;
 }
 
-static int change_property_cmd(struct MPContext *mpctx, struct mp_cmd *cmd,
-                               const char *name, int action, void *arg)
+static int show_property_status(struct MPContext *mpctx, struct mp_cmd *cmd,
+                                const char *name, int r)
 {
     struct MPOpts *opts = mpctx->opts;
     int osd_duration = opts->osd_duration;
@@ -4762,9 +4718,10 @@ static int change_property_cmd(struct MPContext *mpctx, struct mp_cmd *cmd,
     bool msg_osd = auto_osd || (on_osd & MP_ON_OSD_MSG);
     int osdl = msg_osd ? 1 : OSD_LEVEL_INVISIBLE;
 
-    int r = mp_property_do(name, action, arg, mpctx);
     if (r == M_PROPERTY_OK || r == M_PROPERTY_UNAVAILABLE) {
         show_property_osd(mpctx, name, on_osd);
+        if (r == M_PROPERTY_UNAVAILABLE)
+            return -1;
     } else if (r == M_PROPERTY_UNKNOWN) {
         set_osd_msg(mpctx, osdl, osd_duration, "Unknown property: '%s'", name);
         return -1;
@@ -4776,6 +4733,88 @@ static int change_property_cmd(struct MPContext *mpctx, struct mp_cmd *cmd,
     return 0;
 }
 
+static int change_property_cmd(struct MPContext *mpctx, struct mp_cmd *cmd,
+                               const char *name, int action, void *arg)
+{
+    int r = mp_property_do(name, action, arg, mpctx);
+    return show_property_status(mpctx, cmd, name, r);
+}
+
+static bool compare_values(struct m_option *type, void *a, void *b)
+{
+    // Since there is no m_option_equals() or anything similar, we convert all
+    // values to a common, unambiguous representation - strings.
+    char *as = m_option_print(type, a);
+    char *bs = m_option_print(type, b);
+    bool res = bstr_equals(bstr0(as), bstr0(bs)); // treat as "" on failure
+    talloc_free(as);
+    talloc_free(bs);
+    return res;
+}
+
+static int cycle_values_cmd(struct MPContext *mpctx, struct mp_cmd *cmd)
+{
+    int first = 0, dir = 1;
+
+    if (strcmp(cmd->args[first].v.s, "!reverse") == 0) {
+        first += 1;
+        dir = -1;
+    }
+
+    const char *name = cmd->args[first].v.s;
+    first += 1;
+
+    if (first >= cmd->nargs) {
+        MP_ERR(mpctx, "cycle-values command does not have any value arguments.\n");
+        return -1;
+    }
+
+    struct m_option prop = {0};
+    int r = mp_property_do(name, M_PROPERTY_GET_TYPE, &prop, mpctx);
+    if (r <= 0)
+        return show_property_status(mpctx, cmd, name, r);
+
+    union m_option_value curval = {0};
+    r = mp_property_do(name, M_PROPERTY_GET, &curval, mpctx);
+    if (r <= 0)
+        return show_property_status(mpctx, cmd, name, r);
+
+    // Find the current value. Note that we even though compare_values() uses
+    // strings internally, we need to convert the cycle-values arguments to
+    // native anyway to "normalize" the value for comparison.
+    int current = -1;
+    for (int n = first; n < cmd->nargs; n++) {
+        union m_option_value val = {0};
+        if (m_option_parse(mpctx->log, &prop, bstr0(name),
+                           bstr0(cmd->args[n].v.s), &val) <= 0)
+            continue;
+
+        if (compare_values(&prop, &curval, &val))
+            current = n;
+
+        m_option_free(&prop, &val);
+
+        if (current >= 0)
+            break;
+    }
+
+    m_option_free(&prop, &curval);
+
+    if (current >= 0) {
+        current += dir;
+        if (current < first)
+            current = cmd->nargs - 1;
+        if (current >= cmd->nargs)
+            current = first;
+    } else {
+        MP_VERBOSE(mpctx, "Current value not found. Picking default.\n");
+        current = dir > 0 ? first : cmd->nargs - 1;
+    }
+
+    return change_property_cmd(mpctx, cmd, name, M_PROPERTY_SET_STRING,
+                               cmd->args[current].v.s);
+}
+
 int run_command(struct MPContext *mpctx, struct mp_cmd *cmd, struct mpv_node *res)
 {
     struct command_ctx *cmdctx = mpctx->command_ctx;
@@ -4952,32 +4991,8 @@ int run_command(struct MPContext *mpctx, struct mp_cmd *cmd, struct mpv_node *re
                                    M_PROPERTY_MULTIPLY, &cmd->args[1].v.d);
     }
 
-    case MP_CMD_CYCLE_VALUES: {
-        char **args = talloc_zero_array(NULL, char *, cmd->nargs + 1);
-        for (int n = 0; n < cmd->nargs; n++)
-            args[n] = cmd->args[n].v.s;
-        int first = 1, dir = 1;
-        if (strcmp(args[0], "!reverse") == 0) {
-            first += 1;
-            dir = -1;
-        }
-        int *ptr = get_cmd_cycle_counter(mpctx, &args[first - 1]);
-        int count = cmd->nargs - first;
-        int r = 0;
-        if (ptr && count > 0) {
-            *ptr = *ptr < 0 ? (dir > 0 ? 0 : -1) : *ptr + dir;
-            if (*ptr >= count)
-                *ptr = 0;
-            if (*ptr < 0)
-                *ptr = count - 1;
-            char *property = args[first - 1];
-            char *value = args[first + *ptr];
-            r = change_property_cmd(mpctx, cmd, property, M_PROPERTY_SET_STRING,
-                                    value);
-        }
-        talloc_free(args);
-        return r;
-    }
+    case MP_CMD_CYCLE_VALUES:
+        return cycle_values_cmd(mpctx, cmd);
 
     case MP_CMD_FRAME_STEP:
         if (!mpctx->playback_initialized)