bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction

bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction

[Thomas Fitzsimmons]

[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]

For Excorporate's diary integration I had to add some advice to work
around the side effects of a diary-make-entry workaround in
icalendar--add-diary-entry, and side effects of diary-make-entry itself.
I'm filing this bug report to try to eliminate the need for any of these
workarounds.

diary-make-entry adds a trailing space to its entry.
icalendar--add-diary-entry works around this by deleting the trailing
space.  It saves the window excursion, but diary-make-entry still leaves
the diary file where (other-buffer (current-buffer)) will return it,
which is a usability bug.

The attached patch, icalendar-diary-make-entry-fix-1.patch, adds
omit-trailing-space and do-not-show parameters to diary-make-entry to
allow it to operate more like a library function and less like an
interactive function.

To keep the code mostly the same (so that I don't need to factor out
another function), I've changed the original logic by adding a
with-current-buffer wrapper, as shown in simplified form in
diary-make-entry-with-current-buffer.patch.  I'm hoping this keeps the
default diary-make-entry logic exactly the same, but I'd like
confirmation from someone more familiar with the subtleties of window
and buffer manipulation.

Thanks,
Thomas


[-- Attachment #2: icalendar-diary-make-entry-fix-1.patch --]
[-- Type: text/x-diff, Size: 3547 bytes --]

diff --git a/lisp/calendar/diary-lib.el b/lisp/calendar/diary-lib.el
index 1be2a05eee..848ac224fb 100644
--- a/lisp/calendar/diary-lib.el
+++ b/lisp/calendar/diary-lib.el
@@ -2062,27 +2062,34 @@ diary-remind
 ;;; Diary insertion functions.
 
 ;;;###cal-autoload
-(defun diary-make-entry (string &optional nonmarking file)
+(defun diary-make-entry (string &optional nonmarking file omit-trailing-space
+                                do-not-show)
   "Insert a diary entry STRING which may be NONMARKING in FILE.
 If omitted, NONMARKING defaults to nil and FILE defaults to
-`diary-file'."
-  (let ((pop-up-frames (or pop-up-frames (window-dedicated-p))))
-    (find-file-other-window (or file diary-file)))
-  (when (eq major-mode (default-value 'major-mode)) (diary-mode))
-  (widen)
-  (diary-unhide-everything)
-  (goto-char (point-max))
-  (when (let ((case-fold-search t))
-          (search-backward "Local Variables:"
-                           (max (- (point-max) 3000) (point-min))
-                           t))
-    (beginning-of-line)
-    (insert "\n")
-    (forward-line -1))
-  (insert
-   (if (bolp) "" "\n")
-   (if nonmarking diary-nonmarking-symbol "")
-   string " "))
+`diary-file'.  If OMIT-TRAILING-SPACE is non-nil, then do not add
+a trailing space to the entry.  If DO-NOT-SHOW is non-nil, do not
+show the diary buffer."
+  (with-current-buffer
+      (let ((diary-file-name (or file diary-file)))
+        (if do-not-show
+            (find-file-noselect diary-file-name)
+          (let ((pop-up-frames (or pop-up-frames (window-dedicated-p))))
+            (find-file-other-window diary-file-name))))
+    (when (eq major-mode (default-value 'major-mode)) (diary-mode))
+    (widen)
+    (diary-unhide-everything)
+    (goto-char (point-max))
+    (when (let ((case-fold-search t))
+            (search-backward "Local Variables:"
+                             (max (- (point-max) 3000) (point-min))
+                             t))
+      (beginning-of-line)
+      (insert "\n")
+      (forward-line -1))
+    (insert
+     (if (bolp) "" "\n")
+     (if nonmarking diary-nonmarking-symbol "")
+     string (if omit-trailing-space "" " "))))
 
 ;;;###cal-autoload
 (defun diary-insert-entry (arg &optional event)
diff --git a/lisp/calendar/icalendar.el b/lisp/calendar/icalendar.el
index a8fd765129..17316ddbbd 100644
--- a/lisp/calendar/icalendar.el
+++ b/lisp/calendar/icalendar.el
@@ -2502,20 +2502,10 @@ icalendar--add-diary-entry
     (when summary
       (setq non-marking
             (y-or-n-p (format "Make appointment non-marking? "))))
-    (save-window-excursion
-      (unless diary-filename
-        (setq diary-filename
-              (read-file-name "Add appointment to this diary file: ")))
-      ;; Note: diary-make-entry will add a trailing blank char.... :(
-      (funcall (if (fboundp 'diary-make-entry)
-                   'diary-make-entry
-                 'make-diary-entry)
-               string non-marking diary-filename)))
-  ;; Würgaround to remove the trailing blank char
-  (with-current-buffer (find-file diary-filename)
-    (goto-char (point-max))
-    (if (= (char-before) ? )
-        (delete-char -1)))
+    (unless diary-filename
+      (setq diary-filename
+            (read-file-name "Add appointment to this diary file: ")))
+    (diary-make-entry string non-marking diary-filename t t))
   ;; return diary-filename in case it has been changed interactively
   diary-filename)
 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: diary-make-entry-with-current-buffer.patch --]
[-- Type: text/x-diff, Size: 954 bytes --]

diff --git a/lisp/calendar/diary-lib.el b/lisp/calendar/diary-lib.el
index 1be2a05eee..c88d04ab21 100644
--- a/lisp/calendar/diary-lib.el
+++ b/lisp/calendar/diary-lib.el
@@ -2066,8 +2066,9 @@ diary-make-entry
   "Insert a diary entry STRING which may be NONMARKING in FILE.
 If omitted, NONMARKING defaults to nil and FILE defaults to
 `diary-file'."
-  (let ((pop-up-frames (or pop-up-frames (window-dedicated-p))))
-    (find-file-other-window (or file diary-file)))
+  (with-current-buffer
+      (let ((pop-up-frames (or pop-up-frames (window-dedicated-p))))
+        (find-file-other-window (or file diary-file)))
   (when (eq major-mode (default-value 'major-mode)) (diary-mode))
   (widen)
   (diary-unhide-everything)
@@ -2082,7 +2083,7 @@ diary-make-entry
   (insert
    (if (bolp) "" "\n")
    (if nonmarking diary-nonmarking-symbol "")
-   string " "))
+   string " ")))
 
 ;;;###cal-autoload
 (defun diary-insert-entry (arg &optional event)

bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction

[Ulf Jasper]

Hi Thomas,

thanks for the patch(es)!

Am 08.05.2019 um 23:40 (-0400) schrieb Thomas Fitzsimmons:

> The attached patch, icalendar-diary-make-entry-fix-1.patch, adds
> omit-trailing-space and do-not-show parameters to diary-make-entry to
> allow it to operate more like a library function and less like an
> interactive function.

This patch (icalendar-diary-make-entry-fix-1.patch) looks good to me,
particulary from icalendar's point of view.  Unit tests are not
affected.

> To keep the code mostly the same (so that I don't need to factor out
> another function), I've changed the original logic by adding a
> with-current-buffer wrapper, as shown in simplified form in
> diary-make-entry-with-current-buffer.patch.  I'm hoping this keeps the
> default diary-make-entry logic exactly the same, but I'd like
> confirmation from someone more familiar with the subtleties of window
> and buffer manipulation.

The other patch file (diary-make-entry-with-current-buffer.patch) need
not be applied.

Maybe someone could confirm that the diary and the window/buffer things
are ok.  I could then apply the patch.

bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction

[Noam Postavsky]

Thomas Fitzsimmons <fitzsim@fitzsim.org> writes:

> To keep the code mostly the same (so that I don't need to factor out
> another function), I've changed the original logic by adding a
> with-current-buffer wrapper, as shown in simplified form in
> diary-make-entry-with-current-buffer.patch.  I'm hoping this keeps the
> default diary-make-entry logic exactly the same, but I'd like
> confirmation from someone more familiar with the subtleties of window
> and buffer manipulation.

> -  (let ((pop-up-frames (or pop-up-frames (window-dedicated-p))))
> -    (find-file-other-window (or file diary-file)))
> +  (with-current-buffer
> +      (let ((pop-up-frames (or pop-up-frames (window-dedicated-p))))
> +        (find-file-other-window (or file diary-file)))

If you're asking whether

    (progn (find-file-other-window (or file diary-file))
      BODY)

is the same as

    (with-current-buffer (find-file-other-window (or file diary-file))
      BODY)

Then yes, I'd say you're fine (assuming BODY doesn't change buffers,
which I believe is the case here).

bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction

[Thomas Fitzsimmons]

Noam Postavsky <npostavs@gmail.com> writes:

> Thomas Fitzsimmons <fitzsim@fitzsim.org> writes:
>
>> To keep the code mostly the same (so that I don't need to factor out
>> another function), I've changed the original logic by adding a
>> with-current-buffer wrapper, as shown in simplified form in
>> diary-make-entry-with-current-buffer.patch.  I'm hoping this keeps the
>> default diary-make-entry logic exactly the same, but I'd like
>> confirmation from someone more familiar with the subtleties of window
>> and buffer manipulation.
>
>> -  (let ((pop-up-frames (or pop-up-frames (window-dedicated-p))))
>> -    (find-file-other-window (or file diary-file)))
>> +  (with-current-buffer
>> +      (let ((pop-up-frames (or pop-up-frames (window-dedicated-p))))
>> +        (find-file-other-window (or file diary-file)))
>
> If you're asking whether
>
>     (progn (find-file-other-window (or file diary-file))
>       BODY)
>
> is the same as
>
>     (with-current-buffer (find-file-other-window (or file diary-file))
>       BODY)
>
> Then yes, I'd say you're fine (assuming BODY doesn't change buffers,
> which I believe is the case here).

Yes, that's what I was asking, thanks.

Ulf, the only other feedback I have for icalendar is that
icalendar--add-diary-entry is useful to/used by other packages (e.g.,
Excorporate) despite it being a private function.  What if we added a
public alias, icalendar-add-diary-entry, within this same patch?  Then I
could check for that alias's existence and only enable the workaround
advice for older Emacs versions.

Thomas

bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction

[Ulf Jasper]

Am 23.05.2019 um 22:49 (-0400) schrieb Thomas Fitzsimmons:
>
> Ulf, the only other feedback I have for icalendar is that
> icalendar--add-diary-entry is useful to/used by other packages (e.g.,
> Excorporate) despite it being a private function.  What if we added a
> public alias, icalendar-add-diary-entry, within this same patch?  Then I
> could check for that alias's existence and only enable the workaround
> advice for older Emacs versions.
>

Could you please provide a patch with all the changes we want to make?

Thanks,
Ulf

bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction

[Eli Zaretskii]

> From: Ulf Jasper <ulf.jasper@web.de>
> Date: Mon, 03 Jun 2019 20:30:13 +0200
> Cc: 35645@debbugs.gnu.org, Noam Postavsky <npostavs@gmail.com>
> 
> Am 23.05.2019 um 22:49 (-0400) schrieb Thomas Fitzsimmons:
> >
> > Ulf, the only other feedback I have for icalendar is that
> > icalendar--add-diary-entry is useful to/used by other packages (e.g.,
> > Excorporate) despite it being a private function.  What if we added a
> > public alias, icalendar-add-diary-entry, within this same patch?  Then I
> > could check for that alias's existence and only enable the workaround
> > advice for older Emacs versions.
> >
> 
> Could you please provide a patch with all the changes we want to make?

Ping!  Thomas, could you please provide a patch as Ulf requested?  We
would like to proceed with fixing this issue.

TIA

bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction

[Thomas Fitzsimmons]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Ulf Jasper <ulf.jasper@web.de>
>> Date: Mon, 03 Jun 2019 20:30:13 +0200
>> Cc: 35645@debbugs.gnu.org, Noam Postavsky <npostavs@gmail.com>
>> 
>> Am 23.05.2019 um 22:49 (-0400) schrieb Thomas Fitzsimmons:
>> >
>> > Ulf, the only other feedback I have for icalendar is that
>> > icalendar--add-diary-entry is useful to/used by other packages (e.g.,
>> > Excorporate) despite it being a private function.  What if we added a
>> > public alias, icalendar-add-diary-entry, within this same patch?  Then I
>> > could check for that alias's existence and only enable the workaround
>> > advice for older Emacs versions.
>> >
>> 
>> Could you please provide a patch with all the changes we want to make?
>
> Ping!  Thomas, could you please provide a patch as Ulf requested?  We
> would like to proceed with fixing this issue.

I tried out this approach, mostly to try to preserve
icalendar--add-diary-entry's current default behaviour of showing the
resulting diary buffer.  However, I was wrong about
icalendar-add-diary-entry being called directly; the icalendar entry
point Excorporate calls is icalendar-import-buffer.  So I think the
original patch is fine as-is, as long as Ulf is OK with the change to
icalendar--add-diary-entry's default behaviour, such that it does not
show the modified diary file's buffer (which some third party packages
might rely on).  To determine whether or not to apply the workarounds
I'll check the "arity" of diary-make-entry.

I'll write the change log and push the patch this evening unless I hear
otherwise.

Thanks,
Thomas

bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction

[Thomas Fitzsimmons]

Thomas Fitzsimmons <fitzsim@fitzsim.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Ulf Jasper <ulf.jasper@web.de>
>>> Date: Mon, 03 Jun 2019 20:30:13 +0200
>>> Cc: 35645@debbugs.gnu.org, Noam Postavsky <npostavs@gmail.com>
>>> 
>>> Am 23.05.2019 um 22:49 (-0400) schrieb Thomas Fitzsimmons:
>>> >
>>> > Ulf, the only other feedback I have for icalendar is that
>>> > icalendar--add-diary-entry is useful to/used by other packages (e.g.,
>>> > Excorporate) despite it being a private function.  What if we added a
>>> > public alias, icalendar-add-diary-entry, within this same patch?  Then I
>>> > could check for that alias's existence and only enable the workaround
>>> > advice for older Emacs versions.
>>> >
>>> 
>>> Could you please provide a patch with all the changes we want to make?
>>
>> Ping!  Thomas, could you please provide a patch as Ulf requested?  We
>> would like to proceed with fixing this issue.
>
> I tried out this approach, mostly to try to preserve
> icalendar--add-diary-entry's current default behaviour of showing the
> resulting diary buffer.  However, I was wrong about
> icalendar-add-diary-entry being called directly; the icalendar entry
> point Excorporate calls is icalendar-import-buffer.  So I think the
> original patch is fine as-is, as long as Ulf is OK with the change to
> icalendar--add-diary-entry's default behaviour, such that it does not
> show the modified diary file's buffer (which some third party packages
> might rely on).  To determine whether or not to apply the workarounds
> I'll check the "arity" of diary-make-entry.
>
> I'll write the change log and push the patch this evening unless I hear
> otherwise.

I pushed the fix to master.

Thomas

bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction

[Eli Zaretskii]

> From: Thomas Fitzsimmons <fitzsim@fitzsim.org>
> Cc: Ulf Jasper <ulf.jasper@web.de>,  35645-done@debbugs.gnu.org,  npostavs@gmail.com
> Date: Fri, 07 Jun 2019 21:36:10 -0400
> 
> > I'll write the change log and push the patch this evening unless I hear
> > otherwise.
> 
> I pushed the fix to master.

Thanks.