Compose buffer enhancements

[phil-s]

To any future readers... post-merge I managed to break the meta-data for this PR such that it has been marked as "closed" rather than "merged", and it no longer indicates the commits that it contained. If you wish to review the changes, you can instead use the following URL:

alphapapa:ement.el:2b9725accdaed1913b83b2495d3f901ee3aba892...efb6005

What is this?

A collection of changes providing an alternative UI for message composition which feels more like the traditional IRC client approach of "just typing your message at the in-buffer prompt", but built on top of Ement.el's pre-existing "compose buffer" feature.

A minimal config for enabling the main features is:

;; Make the message-entry keybindings invoke compose-buffer-centric commands.
(setq ement-room-compose-method 'compose-buffer)
;; Make "just typing" start a new message (see the mode docstring for details).
(ement-room-self-insert-mode 1)

The "new" features are all in use in combination as my daily driver, so I encourage anyone who is interested to go ahead and test this.

The following headings summarise the changes.

Choose minibuffer-centric or compose-buffer-centric behaviour

• User option ement-room-compose-method chooses between minibuffer-centric (default) or compose-buffer-centric (new) behaviour.
• Command ement-room-dispatch-new-message starts writing a new message using your chosen ement-room-compose-method. (Bound to RET in room buffers.)
• Command ement-room-dispatch-new-message-alt starts writing a new message using the alternative method. (Bound to M-RET in room buffers.)
• Command ement-room-dispatch-edit-message edits a message using your chosen ement-room-compose-method. (Bound to <insert> in room buffers.)
• Command ement-room-dispatch-reply-to-message replies to a message using your chosen ement-room-compose-method. (Bound to S-RET in room buffers.)

New commands for entering a compose buffer

• ement-room-compose-edit edits a message using a compose buffer.
• ement-room-compose-reply replies to a message using a compose buffer.

New commands within a compose buffer

• ement-room-compose-send-direct sends a message directly from a compose buffer (without the minibuffer). (Bound to C-x C-s in compose buffers.)
• ement-room-compose-abort kills the compose buffer and delete its window. (Bound to C-c C-k in compose buffers.)
• ement-room-compose-abort-no-history does the same without adding to ement-room-message-history. (Equivalent to C-u C-c C-k.)
• ement-room-compose-history-prev-message cycles backwards through ement-room-message-history. (Bound to M-p in compose buffers.)
• ement-room-compose-history-next-message cycles forwards through ement-room-message-history. (Bound to M-n in compose buffers.)
• ement-room-compose-history-isearch-backward initiates an isearch through ement-room-message-history. (Bound to M-r in compose buffers; continue searching with C-r or C-s.)
• ement-room-compose-history-isearch-backward-regexp initiates a regexp isearch through ement-room-message-history. (Bound to C-M-r in compose buffers; continue searching with C-r or C-s.)

Enable "just typing" to start a message

• Global minor mode ement-room-self-insert-mode enables "just typing" to start a message. (This works irrespective of the chosen ement-room-compose-method.)
• User option ement-room-self-insert-commands determines which commands will start a new message when ement-room-self-insert-mode is enabled (defaulting to self-insert-command and yank).
• User option ement-room-self-insert-chars determines which typed characters will start a new message when ement-room-self-insert-mode is enabled (regardless of whether they are bound to self-insert-command).
• User option ement-room-mode-map-prefix-key defines a prefix key for accessing the full ement-room-mode-map when ement-room-self-insert-mode is enabled. (By default this key is DEL.)
• Command ement-room-self-insert-new-message starts composing a new message beginning with the just-typed character.

Dynamic scaling of compose buffer window height

• User option ement-room-compose-buffer-window-auto-height causes dynamic scaling of the compose buffer window height so that the full message is visible at all times.
• User option ement-room-compose-buffer-window-auto-height-min specifies the minimum window height when ement-room-compose-buffer-window-auto-height is enabled.
• User option ement-room-compose-buffer-window-auto-height-max specifies the maximum window height when ement-room-compose-buffer-window-auto-height is enabled.

Miscellaneous compose buffer changes

• User option ement-room-compose-buffer-display-action declares how and where a new compose buffer window should be displayed. (By default, in a new small window below the associated room buffer.)
• Event highlighting for compose buffers -- when editing or replying to a message in a compose buffer, the related room event is highlighted persistently until the compose buffer is killed.
• Improve header-line-format in compose buffers
• Add dabbrev support for compose buffers -- dabbrev will prioritise firstly the associated room, and secondly all other rooms, before looking to other buffers for completions.
• Make user/room-name completion (M-TAB) more reliable in compose buffers.

Miscellaneous other changes

• Defer to ement-room-message-history over kill-ring usage -- aborted messages are added to ement-room-message-history.
• Provide prefix key labels for which-key (and similar) to display for ement-room-mode-map. (Primarily useful for ement-room-mode-map-prefix-key.)
• Prohibit non-interactive editing of edit events.

[phil-s]

Apologies to anyone else receiving email notifications for this; I realise it's been a bit spammy.

...and I just spotted the way to make a github PR into a draft formally, so I've now done that.

[phil-s]

Emacs bug#66792 has caused me so much pain for ement-room-self-insert-mode, but I think the latest push has finally gotten my workaround working the way I'd intended. There were some subtle bugs before, causing unintended contamination of ement-room-mode-map wrt the self-insert-command remappings (you may need to reevaluate that keymap definition or restart Emacs to resolve that in a session running the old code), and the advertised map in the mode help had problems as well, but I think these things are now working correctly.

[phil-s]

I've added compose buffer history navigation on M-p and M-n, which is equivalent to those same bindings when navigating the history in the minibuffer (and should also feel familiar to users of Magit's git-commit.el).

I've also added message history searching: M-r and C-M-r to initiate string and regexp searches respectively; and C-r (or C-s) to continue to an earlier (or later) match. (I.e. again, exactly the same as using these bindings in the minibuffer.)

These are the last features I was planning, so I'll call this feature-complete. I still have a little bit of refactoring to do and I'm aware of a couple of minor bugs to fix, but I figure it'll be ready for review after that.

I think a few people are using this, so now would be a good time to let me know how you're getting on with it, and whether you're experiencing any problems?

[phil-s]

I still have a little bit of refactoring to do and I'm aware of a couple of minor bugs to fix, but I figure it'll be ready for review after that.

I think I'm done. I've just done the last of the refactoring that I could see a need for, and I don't believe I've broken anything in the process. I've rebased over the current master branch. I guess I'll remove the "Draft" status, and open this up for review.

[alphapapa]

Thanks for your work on this. As expected, it is a lot of code (which is likely surprising to those users who think that implementing such a thing is trivial and are surprised that it doesn't already exist).

My first thought is, would it be reasonable to put this code in a separate library? I feel like that would make it easier to review and maintain. It would certainly make it easier for me to understand, keeping its code separate. What do you think?

[phil-s]

I'm happy to take a look at that. I'd had similar thoughts along the way about splitting this out, but didn't want to complicate matters.

(Potentially other parts of ement-room.el could benefit from the same treatment -- a lot of Ement code is in that one file.)

[phil-s]

That said, for review purposes you'd definitely want to look at things commit-by-commit, and not attempt to read the whole PR as a single diff, so I'm not sure moving the code will help with that -- I've tried to make each change atomic, with relevant commit messages. There's also some scope for cherry-picking some of the early commits up front, as there were a handful of little clean-ups which aren't tied to new functionality.

For maintenance purposes, I think having the compose-buffer functionality in a separate library is probably a good thing, but that's going to entail moving some pre-existing code as well as some of the new code (but not all of it -- ement-room-self-insert-mode in particular works with or without the compose-buffer features).

[alphapapa]

Ok, thanks. I want to tag v0.14 for tomorrow, so I'll plan to review and merge this for v0.15. I appreciate your patience, as always.

[phil-s]

These fixup commits are all just documentation -- I'll be sure to comment if I make any functional changes or bug fixes.

Because it's Reviewable I'm also not force-pushing anything at this point, but I can easily squash the current fixups and rebase over 0.14 if that's preferable.

(Ditto for my other PRs.)

[alphapapa]

I'd generally be grateful if they were rebased on top of master and had the minor fixups squashed, yes. Thanks.

[phil-s]

Rebased onto current master.

[phil-s]

I've added a new commit to fix the completion-at-point behaviour in compose buffers.

Previously one could complete names with M-TAB at the start of a compose buffer, but not in the middle of a message.

[phil-s]

Rebased over current master.

[alphapapa]

Rebased over current master.

Thanks. I browsed through the diff quickly, and it looks thorough. I can tell you've put a lot of work into it.

It looks to me like most of the changes are to do with the new compose buffer functionality. I see that there may be some unrelated changes in the branch, like 9002bc5, but I don't know if that's necessarily a problem. (Normally I'd ask that they be split out, but with a branch this substantial, and of this apparent quality, I think I could overlook that. :)

How would you like to proceed from here? Assuming that I have no major objections (which I doubt I will, but I'll go over the code once more), do you want to split any of the commits into separate branches/patches/PRs? Or would you rather I just merge it as-is?

[phil-s]

I see that there may be some unrelated changes in the branch, like 9002bc5, but I don't know if that's necessarily a problem.

Yeah, there are a few commits which were drive-by fixes while I was working on something else, and not specific to the main functionality I was adding, so cherry-picking those up-front, independently of merging the PR, would certainly be an option.

The particular commit you've mentioned is something I'd lifted from #230 though, and there were other changes I thought seemed sensible there, so perhaps we should revisit that one first?

Other than that, the first five commits in this branch might be cherry-picked up front. (I think I'd moved those to the start to make that easy, just in case.)

• ed08bc2 Fix: (ement-room-send-org-filter) Prevent a new frame being created
• a17ffa1 Fix: (ement-room-write-reply) Assertions
• 375744c Tidy: (ement-room-mode) Formatting
• ea99f8d Fix: (ement-room-compose-send) Docstring
• cba68d9 Tidy: Whitespace in README.org

(I also have no problem with all these commits staying in the PR, though.)

How would you like to proceed from here? Assuming that I have no major objections (which I doubt I will, but I'll go over the code once more), do you want to split any of the commits into separate branches/patches/PRs? Or would you rather I just merge it as-is?

I'm not looking to split it into multiple PRs if you don't need me to do that.

If you're essentially happy with the functionality then I think the only other thing to consider is whether or not to shift parts of this into a separate file before merging.

If that's going to happen anyway then there's a slight benefit in terms of VCS history if the commits are being made to the file in which the code is ultimately going to live. Of course it's less work to lift-and-shift things after the existing commits, rather than rewriting that history; but it's probably not too much work to do the rewrite (he says, not having tried it yet :)

However I didn't particularly want to do any such history rewriting before getting at least a provisional green light for the functionality, and also being certain that (a) we wanted to split things out, and (b) what the file naming should be when doing that.

My very early thoughts about what might warrant being separated out from ement-room.el were the specific-to-compose-buffers functionality and also (separately from this PR) "media" functionality (images, audio, and so forth). I'd counted something like 500+ lines for the former, and 300+ lines for the latter. ement-room.el is 5.5K lines, so that's not a huge drop, but it's not insignificant either, and there are probably more possibilities besides (I didn't get around to checking for other candidates).

If we do that, I wondered about naming. We already have ement-room-list.el which isn't any kind of sub-component of ement-room.el but might look like one as soon as there were some; so I pondered ement-room--compose.el and ement-room--media.el with double-hyphens. I've no idea whether that's a naming pattern anyone uses, but it would distinguish them.

[alphapapa]

Well, I decided that it's been long enough. It looks like the quality of this branch is very high, and from some quick testing, it seems to work well. It's very cool the way you implemented the expanding compose buffer. I think that will prove popular with many users. And the other fixes and enhancements you made are much appreciated as well. Thanks, Phil!

[phil-s]

Thank you Adam! I appreciate the compliments, and am very happy to see this work merged.

I was wondering why this PR page wasn't recognising the merge, and then saw that you'd pushed a commit to my branch to fix some formatting errors I made with the readme file, but that commit hadn't been included when merging the branch. I believe you'll want to cherry-pick commit c163659 on master.

While writing this, I tried force-pushing my branch back to the state you merged, and I can see that github has immediately marked the PR as closed.

I firstly pushed https://github.com/phil-s/ement.el/commits/phil-s/compose-readme-fix/ at the state with your additional commit, so you can obtain it from there if you don't have it locally.

[phil-s]

Ah, I should have left it alone -- I now realise that I've put this PR into a weird state for historical purposes. Because I force-pushed my branch to a commit which had been merged, this PR now thinks it contains zero commits. Which is perfect reasonable with hindsight, but definitely not what I was trying to do.

Edit: I don't know if it's possible to reset the state of a PR in that way, but as a workaround I've put this comparison URL for the merge into the description, which basically provides the same info:
alphapapa:ement.el:2b9725accdaed1913b83b2495d3f901ee3aba892...efb6005

[alphapapa]

Well, that is annoying. I think what happened was that after I pushed the commit to your branch, I didn't update forge again, and then I merged what forge showed was the current state of the PR, which was before that commit.

I cherry-picked the readme commit, but it made no difference.

Now it won't let me force push to your branch anymore, probably since the PR is closed. And it won't let me reopen the PR anymore, either. sigh

Well, I guess our git history is canonical, not what GitHub says.