bug#7019: 23.2; [PATCH] fix bugs in diary/appt

bug#7019: 23.2; [PATCH] fix bugs in diary/appt

[Leo]

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

Emacs failed to notice me some appointments (fortunately they are not
important). Here is the finding:

The doc string of diary-list-entries says the return list is sorted,
which is not the case. (add-hook 'diary-list-entries-hook
'diary-sort-entries) is not guaranteed to work unless that is the very
last function to run. Other functions in that hook can modify the
entries too, for example, diary-include-other-diary-files. The first
patch fixes these issues. Another fix will be to fix the doc string to
state that the returned list is not sorted.

appt-make-list replies on sorted diary entries to work. The second patch
explicitly sorts the list before processing.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Respect-the-doc-string-and-return-sorted-diary-entri.patch --]
[-- Type: text/x-diff, Size: 1328 bytes --]

From 1b678c6c0601f500321cfb037467f6d393f0e7eb Mon Sep 17 00:00:00 2001
From: Leo <sdl.web@gmail.com>
Date: Sun, 12 Sep 2010 10:36:11 +0100
Subject: [PATCH 1/2] Respect the doc string and return sorted diary entries

---
 lisp/calendar/diary-lib.el |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lisp/calendar/diary-lib.el b/lisp/calendar/diary-lib.el
index 39354bd..3c748ac 100644
--- a/lisp/calendar/diary-lib.el
+++ b/lisp/calendar/diary-lib.el
@@ -187,7 +187,6 @@ you will probably also want to add `diary-mark-included-diary-files' to
 
      (setq diary-display-function 'diary-fancy-display)
      (add-hook 'diary-list-entries-hook 'diary-include-other-diary-files)
-     (add-hook 'diary-list-entries-hook 'diary-sort-entries)
 
 in your `.emacs' file to cause the fancy diary buffer to be displayed with
 diary entries from various included files, each day's entries sorted into
@@ -789,6 +788,7 @@ LIST-ONLY is non-nil, in which case it just returns the list."
               (goto-char (point-min))
               (run-hooks 'diary-nongregorian-listing-hook
                          'diary-list-entries-hook)
+	      (diary-sort-entries)
               (unless list-only
                 (if (and diary-display-function
                          (listp diary-display-function))
-- 
1.7.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Make-appt-make-list-more-robust.patch --]
[-- Type: text/x-diff, Size: 1080 bytes --]

From 7d29e59fb61a8ef76e1c5c583a52cc25b32fba41 Mon Sep 17 00:00:00 2001
From: Leo <sdl.web@gmail.com>
Date: Sun, 12 Sep 2010 10:41:19 +0100
Subject: [PATCH 2/2] Make appt-make-list more robust

It depends on a list of sorted diary entries to work; so explicitly
sort the entries.
---
 lisp/calendar/appt.el |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/lisp/calendar/appt.el b/lisp/calendar/appt.el
index b590290..53469b0 100644
--- a/lisp/calendar/appt.el
+++ b/lisp/calendar/appt.el
@@ -572,7 +572,8 @@ appointment package (if it is not already active)."
               ;; looking for entries beginning with a time.  If the
               ;; entry begins with a time, add it to the
               ;; appt-time-msg-list.  Then sort the list.
-              (let ((entry-list diary-entries-list)
+              (let ((entry-list (sort (copy-sequence diary-entries-list)
+				      'diary-entry-compare))
                     time-string)
                 ;; Skip diary entries for dates before today.
                 (while (and entry-list
-- 
1.7.2


[-- Attachment #4: Type: text/plain, Size: 5 bytes --]


Leo

bug#7019: 23.2; [PATCH] fix bugs in diary/appt

[Glenn Morris]

Leo wrote:

> The doc string of diary-list-entries says the return list is sorted,

Where does it say that? Oh, I guess you might mean:

  Returns a list of all relevant diary entries found, if any, in order
  by date.

Which is true unless include files are involved. Note it does not mean
that entries are also sorted by time of day. I don't think sorting by
default is the right solution. I might just mention in the docs that
if include files are used, the order will day order for the entries
from any given file, then by the order in which the include files were
processed. Some people might want it that way (eg not sorted by time
of day), and those who don't are probably aready using
diary-sort-entries. The fact that the entries are in day order is
really just a consequence of how the code is implemented, rather than
a deliberate choice.

> (add-hook 'diary-list-entries-hook 'diary-sort-entries) is not
> guaranteed to work unless that is the very last function to run.

OK, I think that should just be pointed out in the docs.
(The manual sort of does this without being explicit.)

> appt-make-list replies on sorted diary entries to work.

Looks like it relies on them being sorted in _day_ order, but not in
_time_ order within any given day. Again, this is true unless include
files are involved. I guess this is why nobody noticed till now. I'll
fix that somehow.

bug#7019: 23.2; [PATCH] fix bugs in diary/appt

[Leo]

On 2010-09-14 01:26 +0100, Glenn Morris wrote:
> Leo wrote:
>
>> The doc string of diary-list-entries says the return list is sorted,
>
> Where does it say that? Oh, I guess you might mean:
>
>   Returns a list of all relevant diary entries found, if any, in order
>   by date.
>
> Which is true unless include files are involved. Note it does not mean
> that entries are also sorted by time of day. I don't think sorting by
> default is the right solution. I might just mention in the docs that
> if include files are used, the order will day order for the entries
> from any given file, then by the order in which the include files were
> processed. Some people might want it that way (eg not sorted by time
> of day), and those who don't are probably aready using
> diary-sort-entries. The fact that the entries are in day order is
> really just a consequence of how the code is implemented, rather than
> a deliberate choice.
>
>> (add-hook 'diary-list-entries-hook 'diary-sort-entries) is not
>> guaranteed to work unless that is the very last function to run.
>
> OK, I think that should just be pointed out in the docs.
> (The manual sort of does this without being explicit.)
>
>> appt-make-list replies on sorted diary entries to work.
>
> Looks like it relies on them being sorted in _day_ order, but not in
> _time_ order within any given day. Again, this is true unless include
> files are involved. I guess this is why nobody noticed till now. I'll
> fix that somehow.

Sorry I should be more explicit. I meant date order in all of them. I
think emacs should try its best not to miss any appointments recorded in
it. For example, diary-list-entries-hook is user level variable, so
anything could happen to it, say someone accidentally adds a function
that shuffles diary-entry-list and that could screw up appt.

Thanks.
Leo

bug#7019: 23.2; [PATCH] fix bugs in diary/appt

[Glenn Morris]

Leo wrote:

> Emacs failed to notice me some appointments (fortunately they are not
> important).

Could you explain precisely how this happened, from `emacs -Q'?

Using diary-number-of-entries > 1 and include files?

bug#7019: 23.2; [PATCH] fix bugs in diary/appt

[Glenn Morris]

I've fixed the only case I can think of, in the emacs-23 branch.

bug#7019: 23.2; [PATCH] fix bugs in diary/appt

[Leo]

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

On 2010-09-15 01:30 +0100, Glenn Morris wrote:
> Leo wrote:
>
>> Emacs failed to notice me some appointments (fortunately they are not
>> important).
>
> Could you explain precisely how this happened, from `emacs -Q'?
>
> Using diary-number-of-entries > 1 and include files?

In the main diary file, I have an appointment on Sunday. In the
included, another one Today. Without sorting, Today's appointment is not
in appt-time-msg-list.

So basically put the attached diary and others files in $HOME and emacs
-q -l test.el and check appt-time-msg-list.


[-- Attachment #2: diary --]
[-- Type: text/plain, Size: 42 bytes --]

Saturday 23:59 Exercise
#include "others"

[-- Attachment #3: others --]
[-- Type: text/plain, Size: 23 bytes --]

Wednesday 23:59 meeting

[-- Attachment #4: test.el --]
[-- Type: application/emacs-lisp, Size: 144 bytes --]

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


Leo

bug#7019: 23.2; [PATCH] fix bugs in diary/appt

[Glenn Morris]

Leo wrote:

>> Using diary-number-of-entries > 1 and include files?
[...]
> (setq diary-number-of-entries 7)

Right, thought so. Should be fixed in emacs-23.

bug#7019: 23.2; [PATCH] fix bugs in diary/appt

[Leo]

On 2010-09-15 08:19 +0100, Glenn Morris wrote:
>>> Using diary-number-of-entries > 1 and include files?
> [...]
>> (setq diary-number-of-entries 7)
>
> Right, thought so. Should be fixed in emacs-23.

Thank you.

Leo