Two issues with text properties in embark-consult

[jakanakaevangeli]

embark-default-action doesn't work with consult-buffer's recent files.
Reverting commit bf6158c seems to fix this,
but I haven't studied the code enough to claim that this is the appropriate
solution.

Another issue is that acting on recent files with embark-act can sometimes
cause an element of recentf-list to get an embark-consult-prefix text
property. Writing such a list to recentf's save file causes an error during a
subsequent emacs startup, because the text-property's value is a circular
structure.

For some reason this only happens to remote recent files and I haven't been
able to reproduce this with emacs -q. The following patch does seem to fix this
but, again, I'm not sure if this fix is correct or if it brakes anything.

diff --git a/embark-consult.el b/embark-consult.el
index 17db1cd..ee7a66b 100644
--- a/embark-consult.el
+++ b/embark-consult.el
@@ -252,6 +252,7 @@ (defun embark-consult-refine-multi-type (target)
 actual type."
   (pcase (get-text-property 0 'consult-multi target)
     (`(,category . ,stripped)
+     (setq stripped (substring stripped))
      (put-text-property 0 1 'embark-consult-prefix (substring target 0 1)
                         stripped)
      (cons category stripped))

[minad]

Okay this should be fixed. put-text-property mutates the original candidate. This is wrong.

[oantolin]

Oh, boy. The first problem is a fairly serious problem with the default action for several Consult commands that prefix their candidates with unicode tofu: namely, for consult-line, consult-buffer, consult-isearch, consult-outline, consult-mark and consult-global-mark.

Since most actions require the candidate without the tofu prefix, embark-consult strips the prefix. But the default action only works correctly with the prefix, so when it is stripped it gets stored in the embark-consult-prefix text property so that for the consult commands in the above list it can be restored after injecting the candidate but prior to running the consult command.

But that commit you linked to makes this strategy fail, since it strips all properties from a candidate prior to injecting it in the minibuffer. I think that commit is a good idea generally speaking. When actions read their input via completing-read the inputs get their property stripped anyway, so it makes sense to say that command actions just cannot rely on text properties and we remove the temptation to use them by stripping them.

I forgot we actually depend on those text-properties in embark-consult. :P

I'll think about the best way to fix this. One way, as you say, would be to revert that commit.

[oantolin]

Note that the first problem only appears if you use embark-act and RET. If you use the other way of running the default action, namely, pressing RET in an embark collect buffer, the tofu is preserved.

I think I should just extend that to embark-act: for the default action the target should be the original untransformed target, and embark-transformer-alist should only get used for non-default actions.

With that change we could remove the embark-consult-prefix text property and the function embark-consult-restore-prefix embark setup hook.

[minad]

Since the default action already gets special treatment, why not handle it differently and avoid stripping the properties for the default action? Maybe you could also special case the default action earlier?

[minad]

I think I should just extend that to embark-act: for the default action the target should be the original untransformed target, and embark-transformer-alist shoulg only get used for non-default actions.

This sounds good. Goes in the direction I considered, but better.

[minad]

I hope we can get rid of the tofus at some point - see also radian-software/selectrum#479 (comment). But it will be a long time until then.

[oantolin]

I wound up not always running the default action on the original untransformed target, only in cases where the target originally came from minibuffer completion (i.e., we are in a minibuffer, or in an embark collect buffer created from a minibuffer or recursively from another embark collect buffer that ultimately came from a minibuffer) and the default action has not been overridden and is still the command that opened the minibuffer in the first place. That sounds complicated, but the code is almost shorter than that description.

With that change the embark-consult package can strip tofu without storing it anywhere.

As always, please test.

[jakanakaevangeli]

Great, congrats, fixes both issues. The only minor problem I see with the current approach is when mixing two similar consult commands that aren't `eq`. Example: From consult-buffer, use embark-act to consult-buffer-other-window to open a recent file in another window. This seems like it would be difficult to fix (we would have to treat find-file-other-windw and consult-buffer-other-window differently somehow) and I'm not really sure what my take on this problem is or whether it is important enough to fix it at all. Personally I'm not affected by it at all and the default embark setup encourages using the simple "o" action for such use cases.

[oantolin]

Oh, good point about consult-buffer and consult-buffer-other-window, @jakanakaevangeli! I think I'm content to leave that unfixed, since as you say the default configuration pushes you towards using o which will run find-file-other-window, switch-to-buffer-other-window or bookmark-jump-other-window as appropriate.

[minad]

The consult-buffer-other-window command is meant more for users who do not rely on Embark or want to bind the command directly to a key, replacing the built-in. With Embark I think it is better to use o instead, so I agree that it is okay to leave it like that.