deadpixi / sam

An updated version of the sam text editor.
Other
436 stars 47 forks source link

New 'command send <keys>' feature doesn't work entirely reliably #35

Closed siebenmann closed 8 years ago

siebenmann commented 8 years ago

The problem is that send literally inserts the keys into the command window, followed by a newline. If you already have a partially typed command that you haven't hit Return on, the sent keys are added on the end of it and may do something quite different than you expect. A multi-line sequence such as the sample binding for C-Enter is especially dangerous, because after the first line errors out the subsequent lines will be interpreted as sam commands instead of eg text to be inserted.

I can see two potential fixes. The simple fix is to refuse to do the send if there is pending text in the command window. The thorough fix would be to directly send the command to sam while inserting the command (and its output) immediately before the in-progress command, using something like the code path that executing menu items uses (since they have the same issue and have solved it).

As a side note, I got samterm and sam to crash and panic while fooling around with send'ing a command while I had bits of text selected in the command window. I can't reproduce it, but the crash messages from samterm are:

samterm: host mesg: count 8250 100x 58x 32x (new file) #0
...ignored
28 6e 65 77 20 66 69 6c 65 29 20 23 30 a 6 2 samterm:panic: host-terminal version mismatch: Resource temporarily unavailable

(Unfortunately systemd ate the core dumps. Thanks, systemd. That won't be happening again but it's too late now for this time around.)

deadpixi commented 8 years ago

Hey @siebenmann

I think I've fixed the panic; please check.

The magic for the "look" and "write" menu items are at a lower level than the new Csend functionality; they have their own Tmesg constants associated with them. They're not sending literal "w" or "/" or whatever to the editor, but binary messages.

The new Csend functionality is designed to emulate the "send" menu item, which really does (and I think always has; I've never touched that part of the code) just append text to the end of the command file with a newline. For example, doing this:

a
hello, world!
B /path/to/file <- highlight this and click send
.

Would result in your file looking like

hello, world
B /path/to/fileB /path/to/file

Which I admit isn't the best, but it's what we have to work with at the moment. Detecting if there's pending text in the command window is also kinda squirrely, in that there can be no pending text in the command window but there still be a half open command (for example, having a multi-line command in progress with more than 100 characters typed in it).

So the panic definitely needs to be fixed (and I believe has been), but the other half of the bug...I'm not sure there's a good way to do it short of modifying the sam binary protocol to do something crazy/awesome. That's a possibility, of course, but it's going to take a while.

Thoughts?

siebenmann commented 8 years ago

I think that the send command should be explicitly documented as being the same as the 'send' menu operation, and the caveats of that mentioned. Right now I feel that the documentation leans far too strongly on implying that your sent characters go straight to the editor and are interpreted directly by it, instead of going through the command window as typed/pasted input.

siebenmann commented 8 years ago

As an update: the new text for this in samrc.5 is nice, but I think it might be better to talk about the 'command window' instead of the 'command file' the way that it currently does. It's true that the sam window for commands is sort of a file, but I don't think most people think of it that way.

(And eg the documentation of the standard bindings in sam.1 calls it the command window.)

deadpixi commented 8 years ago

Agreed. The nomenclature is a little...fluid. At some point I need to get in there and standardize it more.

At any rate, I think everything's good on this front (especially with the new bindable command for write). I'm going to close this ticket; please reopen it if you need to.