Editing and/or deleting messages in lui

[ryuslash]

Hi again,

I would really like an additional lui-edit or lui-replace and also a lui-delete. I'm working on some pull requests for an Emacs Slack client and while Slack is a lot like IRC in it's most basic form, there are some very different features as well. Some of these are the ability to edit and delete messages after they've been sent.

@yuya373 has hacked together the edit/replace functionality and now that I'm trying to add the time-stamp (yuya373/emacs-slack#22) as we discussed in one of my previous issues I seem to have to use some knowledge of lui internals to get this to work, and I don't even have it working 100%.

It seems to me that the low-level edit/replace and delete functions would be better off in lui than anywhere else.

I wanted to discuss this before I started writing code, to see if everyone agrees with my ideas, so that's what this issue is for, I'd be happy to try to implement it and create a pull request when we agree on the best way to do it.

My first thought was that lui-insert should add an id or something similar as a text property across the inserted message, so it can easily be found. emacs-slack currently uses the time-stamp from Slack to do this, but I don't think that's very general. I was thinking of adding a buffer-local value that just counts up every time lui-insert adds a message, since I don't think it's necessary to retain uniqueness across sessions or across rooms.

I think the api might look something like this then:

(defun (lui-replace id replacement &optional not-tracked-p) ...)
(defun (lui-delete id &optional not-tracked-p) ...)

I'm not entirely certain that tracking these changes is or isn't a good idea. I can imagine how an edited message would be nice to know about, but a deleted message seems more confusing than anything else.

We also need a way to maintain whether the time-stamp should be displayed or not, depending on lui-time-stamp-only-when-changed-p. I do this in my emacs-slack pull request by adding a text property with the previous time stamp and let-bind that to lui-time-stamp-last when a message is replaced. Another option would be to have a function that can easily select a message relative to another (or the current) one.

Please let me know what you think.

[jorgenschaefer]

Hello! Great ideas, thank you for this issue! Indeed, lui is pretty centered around sequential insertion … editing is a good idea, though. A few notes:

• The id for lui messages needs some thought. Client programs need a way of knowing about that id, so they can reference it later. How does slack know which message to replace? How does the protocol tell the client about that? The protocol has to have a reference to earlier messages, too. So we can just re-use that reference here.
• Timestamps to be displayed, if any, should likely be added as a text property to the lui message (alongside the id). This would mean that replacing a message later can just check for this timestamp property and re-use it if present.
• I'm not sure if modification or deletion should cause a tracking notification, ever. Displaying either is also tricky. How does the slack web client display those?

[ryuslash]

The id for lui messages needs some thought. Client programs need a way of knowing about that id, so they can reference it later. How does slack know which message to replace? How does the protocol tell the client about that? The protocol has to have a reference to earlier messages, too. So we can just re-use that reference here.

Right now emacs-slack adds a text property containing the time stamp that Slack sends along with each message and uses this to identify a certain message in the buffer. Commands that work on messages look at the value of that text property at point when invoked. According to the documentation, the time stamp that Slack provides is used as the identifier for a message. I don't see any reference to any other messages in the data that is sent when a message is updated.

Timestamps to be displayed, if any, should likely be added as a text property to the lui message (alongside the id). This would mean that replacing a message later can just check for this timestamp property and re-use it if present.

Sounds easy and clean enough, only we'd run into trouble again if a program arrives where editing a message also change the time stamp. I don't know how likely that is though.

Another concern might be that when a message is deleted, the message following it might need a time stamp if it didn't show one before.

I'm not sure if modification or deletion should cause a tracking notification, ever. Displaying either is also tricky. How does the slack web client display those?

It seems like emacs-slack doesn't support deleted messages yet, but I imagine that they'd just disappear. Edited messages get an extra "edited_at: 2016-01-09 12:14:16" in their "header" (currently emacs-slack shows a header and body on different lines, but I want to make that customizable in the future, where I plan to show it in a different face under the message itself).

For a bit more crazy idea I thought it might be cool to be able to show edited and deleted messages with a different background color, where deleted messages disappear after a timeout after the relevant buffer is shown. I was even thinking it might be cool to have the background color fade over a little bit of time a-la beacon, but that is not a requirement of course.

[jorgenschaefer]

If time stamps are used to identify messages, that's the best identifier, then, I'd say. I only now realized that the user can edit a message, too. The UI for that will be interesting!

we'd run into trouble again if a program arrives where editing a message also change the time stamp

In the worst case, that would require re-ordering … I don't want to go down that path. A simple solution would be to just not override a time stamp in the inserted text if any is present, though.

when a message is deleted, the message following it might need a time stamp if it didn't show one before

Good point. That one's tricky. I feel a bit iffy about deleted messages to begin with. Might be better to hide them by default and mark them with a dimmed face (like lui-fools).

Edited messages get an extra "edited_at: 2016-01-09 12:14:16" in their "header"

I have a few times now felt like Circe could benefit by a better way of adding metadata to messages that are easily displayed to the user. Something like a tooltip that can be shown for messages including information that is not important, but might be useful. That might be useful here, too.

[ryuslash]

If time stamps are used to identify messages, that's the best identifier, then, I'd say. I only now realized that the user can edit a message, too. The UI for that will be interesting!

That would have to be customizable then, since that will work fine for Slack, but not necessarily for other programs/protocols.

The UI right now is a buffer pops up with the current message, like editing messages in po-mode or source code blocks in org-mode.

we'd run into trouble again if a program arrives where editing a message also change the time stamp

In the worst case, that would require re-ordering … I don't want to go down that path. A simple solution would be to just not override a time stamp in the inserted text if any is present, though.

I wasn't even thinking about reordering, just changing the time stamp. I can't currently think of any application that would do that anyway, so this may not be a relevant concern.

when a message is deleted, the message following it might need a time stamp if it didn't show one before

Good point. That one's tricky. I feel a bit iffy about deleted messages to begin with. Might be better to hide them by default and mark them with a dimmed face (like lui-fools).

I haven't had the pleasure of seeing anything with lui-fools yet, but that does sound like a nice idea to me. This makes deleted messages a lot more visible and less confusing than just removing it from the buffer. A full reload of the buffer would then actually remove the message from the buffer. I don't know what others might think of it, though, they might want deleted messages to be fully removed anyway.

I have a few times now felt like Circe could benefit by a better way of adding metadata to messages that are easily displayed to the user. Something like a tooltip that can be shown for messages including information that is not important, but might be useful. That might be useful here, too.

That would be excellent, Slack has "reactions" as well, with which you can show your emotional reaction to a message in the form of one or more emoji. These emoji are then shown with a counter to show how many people had that reaction to a message. This is an alternative to sending a new message in reply.

Again I feel like this should offer several options as I think that different people will have different opinions about how they want to see this metadata and other protocols might again have metadata that that is important, just not part of the message itself.

[jorgenschaefer]

The UI right now is a buffer pops up with the current message, like editing messages in po-mode or source code blocks in org-mode.

Sounds nice.

So this discussion boils down to something like this:

• Add functions lui-forward|backward-message to navigate between full inserted messages
• Add function lui-replace-message that replaces the message at point
• Add function lui-replace that replaces a message according to a given search criterion (using the functions above). This would keep timestamps intact by using a stored property (but not overriding it if the new message has it already).

An unrelated change would be to add some kind of metadata display. This should probably re-use some kind of tool tip functionality already in Emacs. The features above can be implemented without this feature, though.

Does that sound accurate?

[ryuslash]

May I suggest we make that the following functions and commands:

• lui-forward|backward-message, as you say, to navigate between full inserted messages.
• lui-replace-message which replaces a specified message.
• lui-delete-message which deletes a specified message.
• lui-goto-message to navigate to a specific message.

A message would in each case be specified as an ID, or optionally predicate function. Interactively the lui-replace-message and lui-delete-message would default to the message at point.

I know you don't like the idea of the delete command, but we need something like it anyway, even if the (default) implementation just dims the message to show it's no longer relevant.

I think it would be easiest to add a dynamic variable lui-message-id or similar so like with lui-time-stamp the program can specify which id to use. I think this should be nil by default so that modes like Circe, which don't want to replace or delete messages don't have to worry about it. Although any random unique value should be possible for modes where it only matters for navigation purposes.

I also wonder if we are or aren't going to provide a method of providing arbitrary text properties to be added to lui-insert or not.

I would also like to bring up the idea of documentation. Currently the public api of lui is pretty self-explanatory, however we seem to be moving towards a more complex api and perhaps it would be good to start explaining it some more as we implement the features we're talking about here. I was wondering if you're very partial to the idea of doing this on the wiki, or if perhaps you would find the idea of a Texinfo document appealing. Personally I like writing Texinfo a bit, though I don't much care for maintaining a Texinfo document, I do really like reading one. Another option could be Sphinx, which I think also has a Texinfo exporter (though not as nice as a real Texinfo document) and has the added bonus of allowing us to automatically deploy such documentation to something like Readthedocs if you're interested in such things.

[jorgenschaefer]

I'd really like to separate the API from interactive functions here. What happens when a user wants to edit a message, and if that is possible at all, depends greatly on the application – the user interface really has to be defined there, I believe. (I'm not 100% set on this, so open to ideas.)

I also do think that the lui-*-message functions should all operate similarly on point. Hence why I went for lui-replace in the proposal to add a higher-level function (the omission of lui-delete was accidental and not intentional).

At the same time, I think I'd move lui-time-stamp-time and lui-time-stamp-zone to text properties. lui-insert already uses text properties to add meta information about the inserted text. Then, lui-message-id can be similarly set via text properties.

We could change the signature of lui-insert to (lui-insert str &rest properties) and turn not-tracked-p into another text property. This would make it easier to add properties where needed. Though that's just a trivial wrapper around (lui-insert (propertize str properties...)).

And yes, if we write documentation in anything but the README.md, I'd strongly prefer Sphinx over Texinfo. I have some flame-worthy opinions about Texinfo. ;-)

[ryuslash]

I'd really like to separate the API from interactive functions here. What happens when a user wants to edit a message, and if that is possible at all, depends greatly on the application – the user interface really has to be defined there, I believe. (I'm not 100% set on this, so open to ideas.)

That was my first thought too, but since you mentioned using point I thought maybe you were talking about interactive functions, and then I remembered that lui also provides interactive commands for going through input history so I thought it could be a nice idea to do this in lui. Whether it has any effect outside of your personal buffer is another matter. Either way I'm fine with it.

I also do think that the lui-*-message functions should all operate similarly on point. Hence why I went for lui-replace in the proposal to add a higher-level function (the omission of lui-delete was accidental and not intentional).

You're right, that's probably better. So the only addition I'd like to propose is lui-goto-message.

At the same time, I think I'd move lui-time-stamp-time and lui-time-stamp-zone to text properties. lui-insert already uses text properties to add meta information about the inserted text. Then, lui-message-id can be similarly set via text properties.

We could change the signature of lui-insert to (lui-insert str &rest properties) and turn not-tracked-p into another text property. This would make it easier to add properties where needed. Though that's just a trivial wrapper around (lui-insert (propertize str properties...)).

Sounds good to me!

And yes, if we write documentation in anything but the README.md, I'd strongly prefer Sphinx over Texinfo. I have some flame-worthy opinions about Texinfo. ;-)

I'd like to hear them some time if we see each other on IRC, personally I love reading Info documentation more than any other format, but writing it (especially maintaining it afterwards) is a pain. So Sphinx it will be.

---

I have one more question. What should the default lui-message-id be like for programs like Circe? lui-goto|forward|backward-message would be easiest to implement as a text property search on lui-message-id I think (as emacs-slack currently does), but if a mode doesn't use lui-message-id that becomes difficult. I currently can't think of a good way to determine message beginnings and ends without such a text property.

[jorgenschaefer]

What should the default lui-message-id be like for programs like Circe?

Good point – I suspect if not specified, it can default to an auto-incrementing integer. That's easiest. And we already need the ability to set a text property on a string unless already specified for time stamps, so that works nicely.

[ryuslash]

Alright, sounds good. I'll start working on this as soon as I can.

[jorgenschaefer]

Thank you!

[ryuslash]

Actually, if you don't mind I'd like to give @yuya373 (who I've invited to join the conversation earlier as he's the developer of emacs-slack and knows more about the protocol and requirements than I do) a chance to read this thread and provide his opinion on what we've discussed as well.

[jorgenschaefer]

The more the merrier :-D

[ryuslash]

I had a thought while I was waiting. Wouldn't it be better/easier after all to just always use an auto-incrementing number as the ID of each message? Otherwise I'd have to also add a way to customize the lui-id-increment function, since for our use case with emacs-slack we don't need to do anything at all to generate the new timestamp, we get it from the server. As long as we can add and access our timestamp text-property, we can do what we need to do.

Just a thought, please let me know what you think. I'll start on this soon, I think we've waited long enough.

[jorgenschaefer]

It's probably a good idea to have a lui-specific identifier for the message, and allow an optional application-specific identifier for the message, so applications can find the message they are interested in again easier, yes.

[ryuslash]

In case anyone is interested, I've been very slowly working on this in my editing branch, or in case anyone wants to review or critique my work so far.

[jorgenschaefer]

Thank you for the update :-)

[ryuslash]

Finally this is done, I think this can be closed now, right? Or were there other things we discussed that still need doing too?

[jorgenschaefer]

Looks good to me :-)