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

* bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction
  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
  1 sibling, 0 replies; 9+ messages in thread
From: Ulf Jasper @ 2019-05-13 17:53 UTC (permalink / raw)
  To: Thomas Fitzsimmons; +Cc: 35645

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.








^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction
  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
  1 sibling, 1 reply; 9+ messages in thread
From: Noam Postavsky @ 2019-05-14  0:13 UTC (permalink / raw)
  To: Thomas Fitzsimmons; +Cc: 35645

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






^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction
  2019-05-14  0:13 ` Noam Postavsky
@ 2019-05-24  2:49   ` Thomas Fitzsimmons
  2019-06-03 18:30     ` Ulf Jasper
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Fitzsimmons @ 2019-05-24  2:49 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: 35645

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





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction
  2019-05-24  2:49   ` Thomas Fitzsimmons
@ 2019-06-03 18:30     ` Ulf Jasper
  2019-06-07  9:21       ` Eli Zaretskii
  0 siblings, 1 reply; 9+ messages in thread
From: Ulf Jasper @ 2019-06-03 18:30 UTC (permalink / raw)
  To: Thomas Fitzsimmons; +Cc: 35645, Noam Postavsky

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






^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction
  2019-06-03 18:30     ` Ulf Jasper
@ 2019-06-07  9:21       ` Eli Zaretskii
  2019-06-07 12:37         ` Thomas Fitzsimmons
  0 siblings, 1 reply; 9+ messages in thread
From: Eli Zaretskii @ 2019-06-07  9:21 UTC (permalink / raw)
  To: Ulf Jasper; +Cc: fitzsim, 35645, npostavs

> 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





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction
  2019-06-07  9:21       ` Eli Zaretskii
@ 2019-06-07 12:37         ` Thomas Fitzsimmons
  2019-06-08  1:36           ` Thomas Fitzsimmons
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Fitzsimmons @ 2019-06-07 12:37 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 35645, npostavs

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





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction
  2019-06-07 12:37         ` Thomas Fitzsimmons
@ 2019-06-08  1:36           ` Thomas Fitzsimmons
  2019-06-08  6:35             ` Eli Zaretskii
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Fitzsimmons @ 2019-06-08  1:36 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 35645-done, npostavs

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





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#35645: Fix icalendar--add-diary-entry/diary-make-entry interaction
  2019-06-08  1:36           ` Thomas Fitzsimmons
@ 2019-06-08  6:35             ` Eli Zaretskii
  0 siblings, 0 replies; 9+ messages in thread
From: Eli Zaretskii @ 2019-06-08  6:35 UTC (permalink / raw)
  To: Thomas Fitzsimmons; +Cc: 35645, npostavs

> 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.





^ permalink raw reply	[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.