bug#43295: 26.1: calc-mode header line [UPDATED PATCH]

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#43295: 26.1: calc-mode header line [UPDATED PATCH]

Boruch Baum
This is summary of related mail on emacs-devel (an updated patch is attached):

On 2020-08-31 14:44, Boruch Baum wrote:

> Back in version 21, emacs introduced a static 'header-line' that could
> be inserted at the top of any buffer. Calc mode is one emacs package
> that does not use it and could benefit from it, so the attached patch
> offers that feature. The main benefit is that the 'calc trail' buffer
> (what some greybeards from the mechanical age would remember as the
> 'tape reel') no longer has its title line scroll off the visible
> window. The patch also includes:
>
> 1) Width-sensitive text for the header line, so that it is readable for
>    very narrow windows, and scales to very wide windows.
>
> 2) Display of the 'calc trail' buffer when invoking calc from a frame
>    that is split vertically (C-x 3, M-x split-window-right).
>
> 3) My version of emacs includes a unicode character at 'C-x 8 <return>
>    POCKET CALCULATOR', that I did not include in the header line as the
>    mode's icon, but that could be done.
>
> The patch was diff'ed against the version of emacs that I have: the
> latest-and-greatest that debian is distributing ... v26.1

On 2020-09-07 14:01, Boruch Baum wrote:

> First, congratulations on assuming your new responsibilities.
>
> On 2020-09-07 17:00, Lars Ingebrigtsen wrote:
> > The patch doesn't apply to Emacs 28, so I've respun it (included below).
>
> Oops. I didn't think there would be a difference. I'm using emacs 26.1
> in debian and I didn't download the v28 calc.el
>
> > This is somewhat inscrutable, and is repeated twice (once for the calc
> > buffer and once for the trail buffer).
> >
> > It just centres whatever the string like "--- this ---", so it seems
> > like it should land in a single function for reuse.
--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

calc.patch (4K) Download Attachment
NEWS.patch (628 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#43295: 26.1: calc-mode header line [UPDATED PATCH]

Lars Ingebrigtsen
Boruch Baum <[hidden email]> writes:

> This is summary of related mail on emacs-devel (an updated patch is
> attached):

Thanks; I applied it to Emacs 28, but it needed some changes because it
didn't apply cleanly (and it didn't set the header in the main calc
buffer?)  But it looks like it works now, at least, but you should
perhaps take a look at the result and see whether it looks like you
imagined...

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#43295: 26.1: calc-mode header line [UPDATED PATCH]

Boruch Baum
On 2020-09-10 23:45, Lars Ingebrigtsen wrote:
> Thanks; I applied it to Emacs 28, but it needed some changes because it
> didn't apply cleanly (and it didn't set the header in the main calc
> buffer?)  But it looks like it works now, at least, but you should
> perhaps take a look at the result and see whether it looks like you
> imagined...

I see a few things. Here are some comments, with reference to a new
patch, attached, which is a diff based upon the savannah-git (my
calc28.el).

1) I notice that I had forgotten to remove two lines of coding notes to
   myself @lines ~1395. They can be removed.

2) You removed two lines I had @lines ~1428, and copied a modified
   version of them to @lines ~2008. I think the absence of the two lines
   ~1428 may be cause the problem that you mentioned.

3) I see an additional problem with your modification to the patch that
   you'll notice upon starting calc with a very narrow window. Your line
   ~2009 reads

                        (* 2 (/ (window-width) 3)) -3))

   which is code for the trail buffer, but it should be for the main
   buffer, like in my line ~1429

+                       (/ (* (window-width) 2) 3) 1)

3) If you modify ~2009, then that snippet might be redundant together
   with my snippet ~1428, but it shouldn't do any harm (but remove the
   comment line 'Added by Lars?').

4) You also did something that I welcome, but that was done at Eli
   Zaretskii's insistence, so you may want to co-ordinate with him about
   it. My very original patch looked like your final result @line ~1419,
   but Eli on-list insisted that was wrong on the basis of 'breaking backward
   compatibility', because in the old behavior, the trail buffer always
   had a title line inside the buffer even when calc-show-banner was
   NIL. My position was that's a bug. That part should really read (modifed
   from my original):

  (if calc-show-banner
    (calc--header-line "Emacs Calculator Trail" "Calc Trail"
                       (/ (window-width) 3) -3)
   (when (zerop (buffer-size))
     (let ((buffer-read-only nil))
       (insert (propertize "Emacs Calculator Trail\n" 'face 'italic))))))


--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

calc28.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#43295: 26.1: calc-mode header line [UPDATED PATCH]

Boruch Baum
On 2020-09-11 14:15, Lars Ingebrigtsen wrote:
> Boruch Baum <[hidden email]> writes:
>
> Could you send a new patch that has all the changes that you want that I
> can just apply?  Adding discussion into the patch itself isn't very
> helpful.  :-)
>
> And please submit the patch in a way that it can just be applied.  This
> patch can't be applied either, since you've changed the name of the file
> to "calc28.el".

I can't know everything to include in the patch until you tell me what
your decisions are about the comments I made, most importantly whether
Eli's decision stands about the old trail buffer behavior, and whether
to include your addition @line ~2000 something.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



Reply | Threaded
Open this post in threaded view
|

bug#43295: 26.1: calc-mode header line [UPDATED PATCH]

Boruch Baum
In reply to this post by Boruch Baum
On 2020-09-11 14:15, Lars Ingebrigtsen wrote:
> Could you send a new patch that has all the changes that you want that I
> can just apply?  Adding discussion into the patch itself isn't very
> helpful.  :-)

In the attached patch:

1) the hunk @1421 restores the behavior that Eli wants

2) the hunk @2010 corrects the width calculation in that case

3) the hunk @2133 better handles the header line for the trail display
--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

calc-003.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#43295: 26.1: calc-mode header line [UPDATED PATCH]

Lars Ingebrigtsen
Boruch Baum <[hidden email]> writes:

> In the attached patch:
>
> 1) the hunk @1421 restores the behavior that Eli wants
>
> 2) the hunk @2010 corrects the width calculation in that case
>
> 3) the hunk @2133 better handles the header line for the trail display

This results in a trail buffer that looks like:



Which surely can't be optimal?  If modified as follows, the duplicated
text disappears:

diff --git a/lisp/calc/calc.el b/lisp/calc/calc.el
index bf8b006d7c..62c0bea6d4 100644
--- a/lisp/calc/calc.el
+++ b/lisp/calc/calc.el
@@ -1396,8 +1396,6 @@ calc--header-line
         (let* ((len-long (length long))
                (len-short (length short))
                (fudge (or fudge 0))
-               ;; fudge for trail is: -3 (added to len-long)
-               ;; (width  ) for trail
                (factor (if (> width (+ len-long fudge)) len-long len-short))
                (size   (max (/ (- width factor) 2) 0))
                (fill (make-string size ?-))
@@ -1428,6 +1426,12 @@ calc-create-buffer
   (set-buffer (get-buffer-create "*Calculator*"))
   (or (derived-mode-p 'calc-mode)
       (calc-mode))
+  (when calc-show-banner
+    (calc--header-line "Emacs Calculator Mode" "Emacs Calc"
+                       (if calc-display-trail
+                           (/ (* (window-width) 2) 3)
+                         (window-width))
+                       1))
   (setq max-lisp-eval-depth (max max-lisp-eval-depth 1000))
   (when calc-always-load-extensions
     (require 'calc-ext))
@@ -2009,8 +2013,8 @@ calc-refresh
  (setq calc-any-selections nil)
  (erase-buffer)
          (when calc-show-banner
-           (calc--header-line  "Emacs Calculator Mode" "Emacs Calc"
-                       (* 2 (/ (window-width) 3)) -3))
+           (calc--header-line "Emacs Calculator Mode" "Emacs Calc"
+                              (window-width) 1))
  (while thing
    (goto-char (point-min))
    (insert (math-format-stack-value (car thing)) "\n")
@@ -2133,29 +2137,32 @@ calc-record
 (defun calc-trail-display (flag &optional no-refresh interactive)
   (interactive "P\ni\np")
   (let ((win (get-buffer-window (calc-trail-buffer))))
-    (if (setq calc-display-trail
-      (not (if flag (memq flag '(nil 0)) win)))
- (if (null win)
-    (progn
-              (if calc-trail-window-hook
-                  (run-hooks 'calc-trail-window-hook)
-                (let ((w (split-window nil (/ (* (window-width) 2) 3) t)))
-                  (set-window-buffer w calc-trail-buffer)))
-              (calc-wrapper
-               (setq overlay-arrow-string calc-trail-overlay
-                     overlay-arrow-position calc-trail-pointer)
-               (or no-refresh
-                   (if interactive
-                       (calc-do-refresh)
-                     (calc-refresh))))))
-      (if win
-  (progn
-    (delete-window win)
-    (calc-wrapper
-     (or no-refresh
- (if interactive
-     (calc-do-refresh)
-   (calc-refresh))))))))
+    (cond
+     ((setq calc-display-trail
+    (not (if flag (memq flag '(nil 0)) win)))
+      (when (null win)
+        (if calc-trail-window-hook
+            (run-hooks 'calc-trail-window-hook)
+          (setq win (split-window nil (/ (* (window-width) 2) 3) t))
+          (set-window-buffer win calc-trail-buffer))
+        (calc-wrapper
+         (setq overlay-arrow-string calc-trail-overlay
+               overlay-arrow-position calc-trail-pointer)
+         (or no-refresh
+             (if interactive
+                 (calc-do-refresh)
+               (calc-refresh)))))
+      (with-current-buffer calc-trail-buffer
+        (when calc-show-banner
+          (calc--header-line "Emacs Calculator Trail" "Calc Trail"
+                             (window-width win) -3))))
+     (win                               ; not calc-display-trail
+      (delete-window win)
+      (calc-wrapper
+       (or no-refresh
+   (if interactive
+       (calc-do-refresh)
+             (calc-refresh)))))))
   calc-trail-buffer)
 
 (defun calc-trail-here ()


--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

attachment0 (20K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#43295: 26.1: calc-mode header line [UPDATED PATCH]

Boruch Baum
On 2020-09-13 15:15, Lars Ingebrigtsen wrote:
> Boruch Baum <[hidden email]> writes:
>
> This results in a trail buffer that looks like:

Which is intentional. This is the 'prior behavior' that I mentioned Eli
didn't want to change so as not to 'break backward compatability'. It's
all in the message history on emacs-devel.

> Which surely can't be optimal?  If modified as follows, the duplicated
> text disappears:

I don't mind. My original submission had no duplicate text. Eli might
mind.

One possible benefit of Eli's position is that someone might want to
save the trail buffer to a file and expect the title string. Another
benefit would be if there exists third-party code written to parse the
buffer expecting to delete the first two lines.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



Reply | Threaded
Open this post in threaded view
|

bug#43295: 26.1: calc-mode header line [UPDATED PATCH]

Lars Ingebrigtsen
Boruch Baum <[hidden email]> writes:

> Which is intentional. This is the 'prior behavior' that I mentioned Eli
> didn't want to change so as not to 'break backward compatability'. It's
> all in the message history on emacs-devel.

I kinda doubt that that's the prior behaviour Eli had in mind.  My
powerful mind reading powers says that he probably meant that in the
"backward compatibility mode", there should be no headers, only text in
the buffer (like before).

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#43295: 26.1: calc-mode header line [UPDATED PATCH]

Eli Zaretskii
In reply to this post by Boruch Baum
> Date: Sun, 13 Sep 2020 10:54:11 -0400
> From: Boruch Baum <[hidden email]>
> Cc: [hidden email], Eli Zaretskii <[hidden email]>
>
> On 2020-09-13 15:15, Lars Ingebrigtsen wrote:
> > Boruch Baum <[hidden email]> writes:
> >
> > This results in a trail buffer that looks like:
>
> Which is intentional. This is the 'prior behavior' that I mentioned Eli
> didn't want to change so as not to 'break backward compatability'. It's
> all in the message history on emacs-devel.

To clarify, I think I wanted the prior behavior when the new one is
disabled.