Fix #639

[tarsius]

Closes #639.

At https://github.com/minad/jinx/issues/140#issuecomment-1978951419 @minad wrote (emphasis mine):

This is a bug in Forge, which will create problems for many packages.

Forge sets that variable to a list beginning with forge--topic-id, so when the value ends up being t, then another package must also be messing with the global value, I saw Gnus mentioned. Anyway, what I meant to ask about is this:

minibuffer-allow-text-properties should only be let-bound around read-from-minibuffer or completing-read,

This pr does that.

or if possible should be set only locally in the minibuffer via minibuffer-setup-hook.

That doesn't seem to work for me.

Also note that completing-read does not necessarily preserve text properties (even with minibuffer-allow-text-properties=t!), which means that code depending on properties during completion is most likely incorrect.

@minad Could you please say more about when this setting may be ignored? I.e., is the third commit in this pull-request necessary just to be on the safe side? Thanks (again ;P (see email)) for your help!

The reason I am passing along the "actual" value using a text property, is, that while I can pass an alist as the collection argument of completing-read, something equivalent doesn't seem to be possible when using programmed completion. Programmed completion is needed both for performance reasons, and to avoid overwhelming the users with too many choices (while allowing them to extend the set of completion candidates, iff needed).

[minad]

Could you please say more about when this setting may be ignored? I.e., is the third commit in this pull-request necessary just to be on the safe side? Thanks (again ;P (see email)) for your help!

It is not even correct to say "ignored". It is not supported in the first place. For example when you type something in the default completion UI and press RET, the minibuffer content is submitted as is without any text properties. I always recommend to test with the default completion UI (without Vertico, Ivy, Helm) to make sure that everything works as expected.

[minad]

The reason I am passing along the "actual" value using a text property, is, that while I can pass an alist as the collection argument of completing-read, something equivalent doesn't seem to be possible when using programmed completion. Programmed completion is needed both for performance reasons, and to avoid overwhelming the users with too many choices (while allowing them to extend the set of completion candidates, iff needed).

It should always be possible to restore the original id by looking up the completed string (returned by completing-read) in an alist. The only requirement is that the candidate strings are unique with respect to equal.

[tarsius]

It is not even correct to say "ignored". It is not supported in the first place. For example when you type something in the default completion UI and press RET, the minibuffer content is submitted as is without any text properties. I always recommend to test with the default completion UI (without Vertico, Ivy, Helm) to make sure that everything works as expected.

I'll have to experiment with that some more. I figured that with require-match t, we would get the original completion choice back; but yes, I probably didn't confirm that actually works with built-in completion.

It should always be possible to restore the original id by looking up the completed string (returned by completing-read) in an alist. The only requirement is that the candidate strings are unique with respect to equal.

I'll do that.

(I previously went with properties because somewhere you said something along the line "we have no choice but to use properties", but I am now guessing that only applies to the inner workings of Vertico, and users of completing-read actually do have a choice.)

[tarsius]

I'll do that.

Done.

Thanks for your valuable insight!

[tarsius]

In f3895cdde24d569e8d037cb8b7bd1a86a9e03627.