Conflict with other comint extensions that use text properties

[joaotavora]

Hi. I recently solved the issue https://github.com/joaotavora/sly/issues/119 in SLY, a Common Lisp IDE that has a REPL based on comint.el. The issue's cause was your package xterm-color.el.

Specifically, the problem happens because your package places a function in comint-preoutput-filter-functions that, by default, wipes out every text property inbound in the input string, even if those properties are "private" or in no way relevant to xterm-color. Such was the case with the sly-mrepl--* properties that were causing the actual problem.

May I suggest that you alter xterm-color.el to only cleanup the properties that you think may indeed be problematic to the "internal state machine" (as per the variable xterm-color-preserve-properties documentation).

Thanks very much, João

[atomontage]

Hi,

I see no issue here on behalf xterm-color.el if configured in the way the documentation describes

(and I'm not really referring to xterm-color-preserve-properties which, as documented, should only really be used with eshell).

From the xterm-color documentation:

;; comint install (progn (add-hook 'comint-preoutput-filter-functions 'xterm-color-filter)

This will place xterm-color-filter at the beginning of the hook list. From the documentation of comint-preoutput-filter-functions:

"List of functions to call before inserting Comint output into the buffer. Each function gets one argument, a string containing the text received from the subprocess. It should return the string to insert, perhaps the same string that was received, or perhaps a modified or transformed string.

The functions on the list are called sequentially, and each one is given the string returned by the previous one. The string returned by the last function is the text that is actually inserted in the redirection buffer."

Since "text received from the subprocess" can not contain text properties, I'm assuming xterm-color-filter wasn't placed _at the beginning of the hook list__ (another filter function could have jumped it) in your setup or something else is breaking the comint contract I just pasted.

So yes, xterm-color-filter will wipe text properties by default but since it should be the first filter function, there shouldn't be any to begin with. Wiping the text properties is a side-effect of the fast path I'm using to process each individual character, it's the best possible option for both performance and correctness.

Best, Chris

On Fri, 11 Aug 2017 08:18:15 +0000 (UTC), João Távora notifications@github.com wrote:

Hi. I recently solved the issue https://github.com/joaotavora/sly/issues/119 in SLY, a Common Lisp IDE that has a REPL based on comint.el. The issue's cause was your package xterm-color.el.

Specifically, the problem happens because your package places a function in comint-preoutput-filter-functions that, by default, wipes out every text property inbound in the input string, even if those properties are "private" or in no way relevant to xterm-color. Such was the case with the sly-mrepl--* properties that were causing the actual problem.

May I suggest that you alter xterm-color.el to only cleanup the properties that you think may indeed be problematic to the "internal state machine" (as per the variable xterm-color-preserve-properties documentation).

Thanks very much, João

[atomontage]

Note that the behavior I previously described can also be triggered by comint-preoutput-filter-functions being made buffer-local and a function that adds text properties being added there.

I think I'll remove the comint-specific configuration bits from the documentation and advise that people configure comint-derived modes (e.g. shell-mode) for xterm-color separately. That way they can make sure that xterm-color-filter is really the first filter that gets applied to the process output (which is really what you want anyway, you don't want to be passing along ANSI state machine bytes which are essentially garbage to the other filters).

Thanks for the report.

Chris

[joaotavora]

Hi Chris,

I understand your argument.

As per the documentation, you are -- almost, and very narrowly -- correct. I say "almost" and "narrowly" because you don't have any way of ensuring your function remains the first in the list.

Also in practice you are mistaken if you assume that comint-output-filter, which is the function that uses comint-preoutput-filter-functions, is only called from comint.el with output from the subprocess. It is not. That function is also called by comint.el users, such as Emacs core's own extensions (ielm.el, after which SLY is modelled, but also gdb.el, ange-ftp.el, and others) , to insert arbitrary text in the command lines. Doing so via this function is a way to emulate the process output.

I might agree if you say SLY is abusing comint.el's doc but then Emacs does, too, and who knows how many third party extensions. I also wouldn't be surprised if your assumption "text received from the subprocess can not contain text properties" is ever violated.

In general I think it is a bad idea to rid objects of characteristics that you don't own. Now I understand that you are reconstituting strings character by character for speed, but I wonder if even in that mechanism text properties have to be sacrificed. What is the speed impact of keeping a list of text properties and reapplying them on the reconstituted stream?

[joaotavora]

Note that the behavior I previously described can also be triggered by comint-preoutput-filter-functions being made buffer-local and a function that adds text properties being added there.

Yes, I didn't even think of that one.

I think I'll remove the comint-specific configuration bits from the documentation and advise that people configure comint-derived modes (e.g. shell-mode) for xterm-color separately.

Thanks, that helps. Let me know if you think it's too drastic and ever change your mind, I'll apply a workaround to SLY.

[atomontage]

I agree that using comint in this fashion on my side isn't really ideal and can lead to breakage since many other modes are built on top of it and have their own expectations. Also conceptually, it doesn't make sense for me to try and deal with pre-existing text properties in the fast path and also there's the performance thing.

So basically I'll remove all references to comint from xterm-color.el documentation, and steer people towards configuring specific modes (but not comint itself) that want to use it, separately. That should cover breakage of the sort you encountered, but of course issues could still arise if people (or things like spacemacs) don't pay attention to the has-to-be-in-front bit and configure the modes wrong. At least they'll be easy to find (localized) and fix.

I'll close this issue when I update the docs which should be soon. Cheers.

[atomontage]

Updated documentation and sample configuration to make this issue clear.