Improvements and bug fixes for the compose buffer enhancements (part 2)

[phil-s]

A handful of bug fixes and enhancements following on from https://github.com/alphapapa/ement.el/pull/270.

• Use a window-parameter for the compose buffer window auto-height cache The old system was usually fine in practice, but this is correct/better.

• Prevent unwanted resizing when cycling through buffers in a window This eliminates a potential pain point, whereby the window resizing might easily be unwanted.

• Support an option for deleting compose buffer windows without them being dedicated This is a way to get the "old" behaviour back (when the windows were not dedicated by default).

• Eliminate the timer-triggered resize from ement-room-compose-buffer-window-buffer-change-handler This no longer seemed necessary, so I took the opportunity to simplify things by removing it.

• Improved code comments.

[phil-s]

I can't tell whether https://github.com/phil-s/ement.el/commit/14ddcd8d18e8b408893a31478bfde1953aed354d would be pointless overkill, as I've only ever used "strongly" dedicated windows (it only just occurred to me today to check the distinction). I can trivially merge this if you think it's a good idea. Or make it a regular variable, if we're dubious about anyone ever needing to change it.

[alphapapa]

Thanks. Quick question from one of the comments:

Perfection would in any case be non-trivial -- consider two compose window side-by-side in a horizontal split, each showing a different compose buffer with a different desired height. We cannot have the "correct" size for both simultaneously. The best thing to do would be to maintain the tallest height amongst all conflicting windows at all times -- but that is, again, considerably more complex.

I don't understand that specific example. I'm envisioning a layout like this:

+-----------+-----------+
| room A    | room B    |
|-----------|-----------|
| compose A | compose B |
+-----------+-----------+

In that case, why couldn't the two compose buffers be at the correct size simultaneously?

[phil-s]

That example was actually for this (less-likely) scenario:

+-----------------------+
| some arbitrary buffer |
|-----------+-----------|
| compose A | compose B |
+-----------+-----------+

I.e. not the simple arrangement we would normally have, but a feasible one nevertheless.

[phil-s]

Phil: I'm still seeing the weird behavior where, when I first open the compose buffer, it shows no lines of text; then if I enter a newline, it shows two lines; then if I backspace one newline, it shows one empty line, as expected. Still wondering if it's just me and my use of "C-x C-M-+" (four times) to increase the font size sometimes that causes it. If it's just me, I'm not too worried about it.

Here are a couple of patches to try, restoring the timer-based resize (which I removed in this PR) in a couple of different ways. I don't know why that's happening, but I suspect this code is what was working around it.

The first version simply restores (with new comments) the code I removed:

diff --git a/ement-room.el b/ement-room.el
index 1eefe74..31ab744 100644
--- a/ement-room.el
+++ b/ement-room.el
@@ -4616,10 +4616,21 @@ ement-room-compose-buffer-window-buffer-change-handler

 See also `ement-room-compose-buffer-window-state-change-handler'."
   (with-selected-window win
-    (when ement-room-compose-buffer-window-auto-height
+    (when (and ement-room-compose-buffer-window-auto-height
+               (window-parameter win 'ement-room-compose-buffer-window-auto-height-cache))
       ;; Clear the auto-height cache for this window.
       (set-window-parameter
-       win 'ement-room-compose-buffer-window-auto-height-cache nil))
+       win 'ement-room-compose-buffer-window-auto-height-cache nil)
+      ;; FIXME: The following has been observed to fix a problem on some
+      ;; systems, with the window initially being the wrong height.  The
+      ;; specific cause is a mystery at present, and it's unclear whether
+      ;; relegating this to the *first* time we see the window is enough,
+      ;; or if this is actually dealing with it on an ongoing basis.
+      (unless (bound-and-true-p ement-room-compose-buffer-window-auto-height-resizing-p)
+        (run-with-timer 0 nil (lambda (win)
+                                (with-selected-window win
+                                  (ement-room-compose-buffer-window-auto-height)))
+                        win)))
     ;; Establish whether we've processed this window before, and whether it was
     ;; created to display a compose buffer.  We set a window property the first
     ;; time that we see the window, so if it's set at all, we've seen it before.

The second version changes it so that the timer runs the first time we process this window, but not subsequently:

diff --git a/ement-room.el b/ement-room.el
index 1eefe74..0ec057f 100644
--- a/ement-room.el
+++ b/ement-room.el
@@ -4638,7 +4638,16 @@ ement-room-compose-buffer-window-buffer-change-handler
                 (delete nil)
                 (t ement-room-compose-buffer-window-dedicated))
           (set-window-dedicated-p
-           win ement-room-compose-buffer-window-dedicated-flag))))))
+           win ement-room-compose-buffer-window-dedicated-flag)))
+      ;; FIXME: The following has been observed to fix a problem on some
+      ;; systems, with the window initially being the wrong height.  The
+      ;; specific cause is a mystery at present.  We do this only the first
+      ;; time we see the window.
+      (unless (bound-and-true-p ement-room-compose-buffer-window-auto-height-resizing-p)
+        (run-with-timer 0 nil (lambda (win)
+                                (with-selected-window win
+                                  (ement-room-compose-buffer-window-auto-height)))
+                        win)))))

 (defun ement-room-compose-buffer-quit-restore-window ()
   "Kill the current compose buffer and deal appropriately with its window.

Without knowing exactly what's going on here, I can't predict whether or not the second version is adequate. If there's any doubt then the first version seems "safer" as it'll trigger in more cases, and there's been plenty of testing with that approach in play.

[alphapapa]

I'm not sure if the problem I described was ever "solved" by any iteration I've tried, so I'd hesitate to merge any code intended to fix it without better understanding the problem. Especially, if the implementation you have now doesn't use a timer, I'd hesitate to restore one that does.

If you're not seeing this problem, and if no one else is reporting it, let's wait and see; especially, since I probably won't use this option as the default for myself. If I really need to, I can investigate it myself or file a bug report about it someday.

Thanks for your help.