all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction
@ 2019-05-09  3:40 Thomas Fitzsimmons
  2019-05-13 17:53 ` Ulf Jasper
  2019-05-14  0:13 ` Noam Postavsky
  0 siblings, 2 replies; 9+ messages in thread
From: Thomas Fitzsimmons @ 2019-05-09  3:40 UTC (permalink / raw)
  To: 35645

[-- 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)

^ permalink raw reply related	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-06-08  6:35 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-09  3:40 bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction Thomas Fitzsimmons
2019-05-13 17:53 ` Ulf Jasper
2019-05-14  0:13 ` Noam Postavsky
2019-05-24  2:49   ` Thomas Fitzsimmons
2019-06-03 18:30     ` Ulf Jasper
2019-06-07  9:21       ` Eli Zaretskii
2019-06-07 12:37         ` Thomas Fitzsimmons
2019-06-08  1:36           ` Thomas Fitzsimmons
2019-06-08  6:35             ` Eli Zaretskii

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.