Suggestion: preserve cur_buffer->find_string when playing a macro

[soveran]

A while ago I modified command.c to preserve the find string when playing a macro, and to this day it has proved useful with no drawbacks. I'm fine with my patch, but I wanted to mention this change in case it sounds useful to others. If that's the case, it could become the standard behavior—or an option if the current behavior is useful for some.

[utoddl]

If I understand you, you've made it so that find arbitrary_string in a macro doesn't update cur_buffer->find_string like it would if you did the same command on the command line.

I can see how it may seem useful. It would avoid the situation where a macro has the unintended side effect of changing cur_buffer->find_string.

However, it also changes the RepeatLast command's behavior, breaking it entirely when playing back a recorded macro containing a Find command and a subsequent RepeatLast command.

We already have a mechanism to avoid side effects from macros. See PushPrefs and PopPrefs. I think it's reasonable to consider whether to extend the function of PushPrefs and PopPrefs to include saving and restoring the current buffer's find_string, last_was_replace, and last_was_regexp. Then you could craft your macros to avoid the issue while preserving the ability to use — and without breaking the semantics of — RepeatLast.

This is an interesting idea. Thanks for the suggestion.

[soveran]

Sorry I didn't share the diff earlier:

diff --git a/src/command.c b/src/command.c
index a00d87b..7eed69b 100644
--- a/src/command.c
+++ b/src/command.c
@@ -503,6 +503,8 @@ void optimize_macro(char_stream *cs, bool verbose) {
 int play_macro(char_stream *cs, int64_t c) {
    if (!cs) return ERROR;

+   char *find_string = str_dup(cur_buffer->find_string);
+
    /* If len is 0 or 1, the character stream does not contain anything. */
    const int64_t len = cs->len;
    if (len < 2 || c < 1) return OK;
@@ -547,6 +549,8 @@ int play_macro(char_stream *cs, int64_t c) {
    free(stream);
    if (--call_depth == 0) executing_macro = false;

+   cur_buffer->find_string = str_dup(find_string);
+
    return stop ? STOPPED : error;
 }

I had not checked what would happen if a macro contains a call to RepeatLast, but I've just tried it and it seems to work fine. I agree that it would be more elegant to extend PushPrefs and PopPrefs. Also, even if you guessed my intentions correctly, I didn't share my motivation: I have some macros that use Find/FindRegExp and I did not like to lose the value of find_string after running them.