GNU bug report logs - #35645
Fix icalendar--add-diary-entry/diary-make-entry interaction

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 35645 in the body.
You can then email your comments to 35645 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
To: bug-gnu-emacs <at> gnu.org
Subject: Fix icalendar--add-diary-entry/diary-make-entry interaction
Date: Wed, 08 May 2019 23:40:30 -0400

[Message part 1 (text/plain, inline)]

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

[icalendar-diary-make-entry-fix-1.patch (text/x-diff, attachment)]

[diary-make-entry-with-current-buffer.patch (text/x-diff, attachment)]

Message #8 received at 35645 <at> debbugs.gnu.org (full text, mbox):

From: Ulf Jasper <ulf.jasper <at> web.de>
To: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Cc: 35645 <at> debbugs.gnu.org
Subject: Re: bug#35645: Fix icalendar--add-diary-entry/diary-make-entry
 interaction
Date: Mon, 13 May 2019 19:53:25 +0200

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.

Message #11 received at 35645 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Cc: 35645 <at> debbugs.gnu.org
Subject: Re: bug#35645: Fix icalendar--add-diary-entry/diary-make-entry
 interaction
Date: Mon, 13 May 2019 20:13:21 -0400

Thomas Fitzsimmons <fitzsim <at> 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).

Message #14 received at 35645 <at> debbugs.gnu.org (full text, mbox):

From: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: Ulf Jasper <ulf.jasper <at> web.de>, 35645 <at> debbugs.gnu.org
Subject: Re: bug#35645: Fix icalendar--add-diary-entry/diary-make-entry
 interaction
Date: Thu, 23 May 2019 22:49:46 -0400

Noam Postavsky <npostavs <at> gmail.com> writes:

> Thomas Fitzsimmons <fitzsim <at> 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

Message #17 received at 35645 <at> debbugs.gnu.org (full text, mbox):

From: Ulf Jasper <ulf.jasper <at> web.de>
To: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Cc: 35645 <at> debbugs.gnu.org, Noam Postavsky <npostavs <at> gmail.com>
Subject: Re: bug#35645: Fix icalendar--add-diary-entry/diary-make-entry
 interaction
Date: Mon, 03 Jun 2019 20:30:13 +0200

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

Message #20 received at 35645 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ulf Jasper <ulf.jasper <at> web.de>
Cc: fitzsim <at> fitzsim.org, 35645 <at> debbugs.gnu.org, npostavs <at> gmail.com
Subject: Re: bug#35645: Fix icalendar--add-diary-entry/diary-make-entry
 interaction
Date: Fri, 07 Jun 2019 12:21:08 +0300

> From: Ulf Jasper <ulf.jasper <at> web.de>
> Date: Mon, 03 Jun 2019 20:30:13 +0200
> Cc: 35645 <at> debbugs.gnu.org, Noam Postavsky <npostavs <at> 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

Message #23 received at 35645 <at> debbugs.gnu.org (full text, mbox):

From: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Ulf Jasper <ulf.jasper <at> web.de>, 35645 <at> debbugs.gnu.org, npostavs <at> gmail.com
Subject: Re: bug#35645: Fix icalendar--add-diary-entry/diary-make-entry
 interaction
Date: Fri, 07 Jun 2019 08:37:18 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Ulf Jasper <ulf.jasper <at> web.de>
>> Date: Mon, 03 Jun 2019 20:30:13 +0200
>> Cc: 35645 <at> debbugs.gnu.org, Noam Postavsky <npostavs <at> 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

Message #28 received at 35645-done <at> debbugs.gnu.org (full text, mbox):

From: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Ulf Jasper <ulf.jasper <at> web.de>, 35645-done <at> debbugs.gnu.org,
 npostavs <at> gmail.com
Subject: Re: bug#35645: Fix icalendar--add-diary-entry/diary-make-entry
 interaction
Date: Fri, 07 Jun 2019 21:36:10 -0400

Thomas Fitzsimmons <fitzsim <at> fitzsim.org> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> From: Ulf Jasper <ulf.jasper <at> web.de>
>>> Date: Mon, 03 Jun 2019 20:30:13 +0200
>>> Cc: 35645 <at> debbugs.gnu.org, Noam Postavsky <npostavs <at> 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

Message #31 received at 35645 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Cc: ulf.jasper <at> web.de, 35645 <at> debbugs.gnu.org, npostavs <at> gmail.com
Subject: Re: bug#35645: Fix icalendar--add-diary-entry/diary-make-entry
 interaction
Date: Sat, 08 Jun 2019 09:35:16 +0300

> From: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
> Cc: Ulf Jasper <ulf.jasper <at> web.de>,  35645-done <at> debbugs.gnu.org,  npostavs <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.