org-review-schedule

org-review-schedule

[Alan Schmitt]

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

Hello,

I've just finished writing a little bit of code that allows the
scheduling of reviews. The basic idea is that every task that is
supposed to be reviewed has a LAST_REVIEW property (a date when the
task / project was last reviewed), and optionally a REVIEW_DELAY
property (with a configurable default value). If the current date is
after LAST_REVIEW + REVIEW_DELAY, then the task is considered up to
review.

I've written a small function to show these tasks in the agenda, and
a sorting function to allow them to be sorted from "this had to be
review so long ago" to "this has just been available to review". Another
function allows to set the LAST_REVIEW property to the current date (or
a chosen date if called with C-u).

I attach the code. It's the first time I'm contributing something like
this, so I don't really know how to do it. The header of the file is
basically the same one as from `contrib/org-expiry.el', and I tried to
keep a similar structure.

Please don't hesitate to comment & criticize the code, I'm still
learning my way around emacs-lisp and org-mode.

Thanks,

Alan


[-- Attachment #2: org-review-schedule.el --]
[-- Type: application/emacs-lisp, Size: 9501 bytes --]

Re: org-review-schedule

[Bastien]

Hi Alan,

thanks for sharing -- some comments:

- you need to update the copyright of the file;

- example code in section 3 of the header is mangled;

- there are some dangling parentheses;

- use (get-text-property (point-min) ...) instead of
  (get-text-property 1 ...)

- I'd use org-review instead of org-review-schedule as prefix;

- maybe you can use "naked" timestamps like 2014-04-19 sam.
  instead of inactive ones, this way using "[" in the agenda
  will not create false positives by inserting entries with
  a REVIEW property.

- I infer from a quick read that this works for the agenda but
  I guess this could work for both the agenda and Org buffers;

Since you took inspirationg from org-expiry, I guess some of
the comments above would apply there too... feel free to hack
into this directions for both org-expiry.el and org-review.el!
Actually, maybe both should be merged somehow, since expiring
is just reviewing entries to interactively delete them.

Thanks for this!

-- 
 Bastien

Re: org-review-schedule

[Alan Schmitt]

Hi Bastien,

Thanks a lot for these, I'll look into them. I have a couple questions
in the meantime.

On 2014-04-19 10:14, Bastien <bzg@gnu.org> writes:

> - maybe you can use "naked" timestamps like 2014-04-19 sam.
>   instead of inactive ones, this way using "[" in the agenda
>   will not create false positives by inserting entries with
>   a REVIEW property.

OK. I'm not sure what "[" is supposed to do in the agenda, and I don't
see how it could interfere. (I like inactive time stamps because I can
easily adjust their dates with C-left and C-right, is it possible to do
so with naked timestamps?)

> - I infer from a quick read that this works for the agenda but
>   I guess this could work for both the agenda and Org buffers;

Yes, clearly.

> Since you took inspirationg from org-expiry, I guess some of
> the comments above would apply there too... feel free to hack
> into this directions for both org-expiry.el and org-review.el!
> Actually, maybe both should be merged somehow, since expiring
> is just reviewing entries to interactively delete them.

Yes. But I should think about it more to see where I could take this.

Thanks again,

Alan

Re: org-review-schedule

[Bastien]

Alan Schmitt <alan.schmitt@polytechnique.org> writes:

>> - maybe you can use "naked" timestamps like 2014-04-19 sam.
>>   instead of inactive ones, this way using "[" in the agenda
>>   will not create false positives by inserting entries with
>>   a REVIEW property.
>
> OK. I'm not sure what "[" is supposed to do in the agenda, and I don't
> see how it could interfere.

It includes headlines with an inactive timestamp in the agenda.
So if you have a property :REVIEW_DELAY: [Inactive timestamp] this
headline will be show in the agenda if the timestamp matches.

> (I like inactive time stamps because I can
> easily adjust their dates with C-left and C-right, is it possible to do
> so with naked timestamps?)

Well... no.  Of course you can simply use inactive timestamps and
advice users not to include them in the agenda.

> Yes. But I should think about it more to see where I could take
> this.

Thanks in advance!

-- 
 Bastien

Re: org-review-schedule

[Alan Schmitt]

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

Hi Bastien,

On 2014-04-19 10:14, Bastien <bzg@gnu.org> writes:

> Hi Alan,
>
> thanks for sharing -- some comments:
>
> - you need to update the copyright of the file;

I changed the date. As I signed the FSF paper, do I need to change the
name as well and put mine?

> - example code in section 3 of the header is mangled;

I removed that example; it should not be in the file.

> - there are some dangling parentheses;

Are those parentheses on a line by themselves? I could not find them.

> - use (get-text-property (point-min) ...) instead of
>   (get-text-property 1 ...)

Changed.

> - I'd use org-review instead of org-review-schedule as prefix;

Changed.

> - maybe you can use "naked" timestamps like 2014-04-19 sam.
>   instead of inactive ones, this way using "[" in the agenda
>   will not create false positives by inserting entries with
>   a REVIEW property.

This is now the case by default, with an option to have inactive or
active time stamps.

> - I infer from a quick read that this works for the agenda but
>   I guess this could work for both the agenda and Org buffers;

It depends what "this" means ;-) My goal was to use it in an agenda
view, but most of the functionality does not depend on it.

> Since you took inspirationg from org-expiry, I guess some of
> the comments above would apply there too... feel free to hack
> into this directions for both org-expiry.el and org-review.el!
> Actually, maybe both should be merged somehow, since expiring
> is just reviewing entries to interactively delete them.

I'm still not sure where to take this ... I agree both are cases of
adding dates to entries and doing things according to those dates, but
I still need to think more about how to generalize it to cover both
cases.

I attach the new version. I would like to propose to add this to the
contrib directory, but I don't know the procedure to submit this code.

Thanks,

Alan


[-- Attachment #2: org-review.el --]
[-- Type: application/emacs-lisp, Size: 7011 bytes --]

Re: org-review-schedule

[Nicolas Goaziou]

Hello,

Alan Schmitt <alan.schmitt@polytechnique.org> writes:

> I changed the date. As I signed the FSF paper, do I need to change the
> name as well and put mine?

For contrib/ directory, you can assign copyright to your name.

> I attach the new version.

Thanks. A few minor comments follow.

> I would like to propose to add this to the contrib directory, but
> I don't know the procedure to submit this code.

You simply copy the file in the contrib/lisp/ directory, edit
contrib/README and edit `org-modules' defcustom in "org.el".

> ;; Example use.
> ;; 

Trailing whitespace.

> ;; 1 - To display the things to review in the agenda.
> ;; 

Ditto.

> (defun org-review-last-planned (last delay)
>   "Computes the next planned review, given the LAST review
>   date (in string format) and the review DELAY (in string
>   format)."
>   (let* ((lt (org-read-date nil t last))
>          (ct (current-time)))

Plain `let' is sufficient here.

>     (time-add lt (time-subtract (org-read-date nil t delay) ct))))
>
> (defun org-review-last-review-prop ()
>   "Return the value of the last review property of the current
> headline."
>   (let ((lr-prop org-review-last-property-name))
>     (org-entry-get (point) lr-prop)))

The `let' seems useless. I think

  (org-entry-get (point) org-review-last-property-name)

is simple enough.

> (defun org-review-toreview-p ()
>   "Check if the entry at point should be marked for review.
> Return nil if the entry does not need to be reviewed. Otherwise return
> the number of days between the past planned review date and today.
>
> If there is no last review date, return nil.
> If there is no review delay period, use `org-review-delay'."
>   (let* ((lr-prop org-review-last-property-name)
>          (lp (org-entry-get (point) lr-prop)))

I suggest to use your own function:


 (let ((lp (org-review-last-review-prop)))
    ...)

>     (when lp 

Trailing whitespace.

>       (let* ((dr-prop org-review-delay-property-name)
>              (dr (or (org-entry-get (point) dr-prop t) 
>                      org-review-delay))

Trailing whitespace.  Also I suggest to merge DR and DR-PROP:

  (let* ((dr (or (org-entry-GET (point) org-review-delay-property-name t)
                org-review-delay))
         ...))

>              (nt (org-review-last-planned lp dr))
>              )

Dangling parenthesis.

>         (if (time-less-p nt (current-time)) nt)))))

This is a matter of taste, but I find one-armed `if' a bit confusing.
Since return value matters, I suggest to use

  (and (time-less-p nt (current-time)) nt)

>
> (defun org-review-insert-last-review (&optional prompt)
>   "Insert the current date as last review. If prefix argument:
> prompt the user for the date."
>   (interactive "P")
>   (let* ((ts (if prompt
>                 (concat "<" (org-read-date) ">")
>               (format-time-string (car org-time-stamp-formats)))))
>     (save-excursion

I don't think this `save-excursion' is needed.

>       (org-entry-put
>        (if (equal (buffer-name) org-agenda-buffer-name)
>            (or (org-get-at-bol 'org-marker)
>                (org-agenda-error))
>          (point))
>        org-review-last-property-name
>        (cond 

Trailing whitespace.

>         ((equal org-review-timestamp-format 'inactive)

`eq' should be used when comparing symbols.

>          (concat "[" (substring ts 1 -1) "]"))
>         ((equal org-review-timestamp-format 'active)

Ditto.

>          ts)
>         (t (substring ts 1 -1)))))))
>
> (defun org-review-skip ()
>   "Skip entries that are not scheduled to be reviewed."
>   (save-restriction
>     (widen)
>     (let ((next-headline (save-excursion (or (outline-next-heading)
>                                              (point-max)))))
>       (cond
>        ((org-review-toreview-p) nil)
>        (t next-headline)))))

This function doesn't move point (so it skips nothing), is it expected?

Also, I think `save-restriction' + `widen' is only useful for
`outline-next-heading'. And `save-restriction' + `save-excursion' +
`widen' = `org-with-wide-buffer'. You may want to rewrite it to
something like:

  (defun org-review-skip ()
    "Skip entries that are not scheduled to be reviewed."
    (and (not (org-review-toreview-p))
         (org-with-wide-buffer (or (outline-next-heading) (point)))))


Regards,

-- 
Nicolas Goaziou

Re: org-review-schedule

[Alan Schmitt]

Hi Nicolas,

Thanks a lot for these very helpful comments. I'll take them into
account. I have a couple questions of my own now.

On 2014-04-25 08:51, Nicolas Goaziou <n.goaziou@gmail.com> writes:

>> I would like to propose to add this to the contrib directory, but
>> I don't know the procedure to submit this code.
>
> You simply copy the file in the contrib/lisp/ directory, edit
> contrib/README and edit `org-modules' defcustom in "org.el".

I guess I should have asked: who decides what goes in contrib? Is this
mature/useful enough to be included? If so, I'll edit these as you
suggest.

>> ;; Example use.
>> ;; 
>
> Trailing whitespace.

After this many violations, I've added

#+begin_src emacs-lisp
  (setq-default show-trailing-whitespace t)
#+end_src

to my configuration ;-)

>> (defun org-review-insert-last-review (&optional prompt)
>>   "Insert the current date as last review. If prefix argument:
>> prompt the user for the date."
>>   (interactive "P")
>>   (let* ((ts (if prompt
>>                 (concat "<" (org-read-date) ">")
>>               (format-time-string (car org-time-stamp-formats)))))
>>     (save-excursion
>
> I don't think this `save-excursion' is needed.

Indeed. I copied this from org-expiry. Looking at the code for
`org-entry-put', I see that it uses `org-with-point' that also uses
save-excursion inside.

>> (defun org-review-skip ()
>>   "Skip entries that are not scheduled to be reviewed."
>>   (save-restriction
>>     (widen)
>>     (let ((next-headline (save-excursion (or (outline-next-heading)
>>                                              (point-max)))))
>>       (cond
>>        ((org-review-toreview-p) nil)
>>        (t next-headline)))))
>
> This function doesn't move point (so it skips nothing), is it
> expected?

It works, so I guess it's not supposed to move the point. It's to be
used with `org-agenda-skip-function', which says:

,-----------------------------------------------------------------------
| Function to be called at each match during agenda construction.       
| If this function returns nil, the current match should not be skipped.
| Otherwise, the function must return a position from where the search  
| should be continued.                                                  
`-----------------------------------------------------------------------

Thanks again,

Alan

Re: org-review-schedule

[Nicolas Goaziou]

Alan Schmitt <alan.schmitt@polytechnique.org> writes:

> I guess I should have asked: who decides what goes in contrib?

The Org maintainer. Another option is to turn it into an ELPA package.

> Is this mature/useful enough to be included? If so, I'll edit these as
> you suggest.

It seems useful. Maturity is not a problem since you have write access
to the repo. Anyway, Bastien will give you the definitive answer.

> After this many violations, I've added
>
> #+begin_src emacs-lisp
>   (setq-default show-trailing-whitespace t)
> #+end_src
>
> to my configuration ;-)

FWIW, I use `whitespace-mode' instead.

> It works, so I guess it's not supposed to move the point. It's to be
> used with `org-agenda-skip-function', which says:
>
> ,-----------------------------------------------------------------------
> | Function to be called at each match during agenda construction.       
> | If this function returns nil, the current match should not be skipped.
> | Otherwise, the function must return a position from where the search  
> | should be continued.                                                  
> `-----------------------------------------------------------------------

Then I suggest to explain it in the docstring (both the return value and
that it will be used as `org-agenda-skip-function' value).


Regards,

-- 
Nicolas Goaziou

Re: org-review-schedule

[Alan Schmitt]

Hi Nicolas,

I've changed all of these, and I will keep testing it over the next few
days. I have one question remaining, though.

On 2014-04-25 08:51, Nicolas Goaziou <n.goaziou@gmail.com> writes:

>>         (if (time-less-p nt (current-time)) nt)))))
>
> This is a matter of taste, but I find one-armed `if' a bit confusing.
> Since return value matters, I suggest to use
>
>   (and (time-less-p nt (current-time)) nt)

Why not use "(when (time-less-p nt (current-time)) nt)" instead of "and"
here?

Thanks,

Alan

Re: org-review-schedule

[Thorsten Jolitz]

Alan Schmitt <alan.schmitt@polytechnique.org> writes:

> Hi Nicolas,
>
> I've changed all of these, and I will keep testing it over the next few
> days. I have one question remaining, though.
>
> On 2014-04-25 08:51, Nicolas Goaziou <n.goaziou@gmail.com> writes:
>
>>>         (if (time-less-p nt (current-time)) nt)))))
>>
>> This is a matter of taste, but I find one-armed `if' a bit confusing.
>> Since return value matters, I suggest to use
>>
>>   (and (time-less-p nt (current-time)) nt)
>
> Why not use "(when (time-less-p nt (current-time)) nt)" instead of "and"
> here?

the answer is in the quote already:

,---------------------------------------------
| Since return value matters, I suggest to use ...
`---------------------------------------------

-- 
cheers,
Thorsten

Re: org-review-schedule

[Nicolas Goaziou]

Thorsten Jolitz <tjolitz@gmail.com> writes:

> the answer is in the quote already:
>
> ,---------------------------------------------
> | Since return value matters, I suggest to use ...
> `---------------------------------------------

Exactly. I use `when' if side-effects are involved or if the code is too
long and doesn't fit nicely in a `and' branch.

Of course, this is nitpicking. There's nothing wrong with `when'
variant.


Regards,

-- 
Nicolas Goaziou

Re: org-review-schedule

[Alan Schmitt]

On 2014-04-26 14:25, Nicolas Goaziou <n.goaziou@gmail.com> writes:

> Thorsten Jolitz <tjolitz@gmail.com> writes:
>
>> the answer is in the quote already:
>>
>> ,---------------------------------------------
>> | Since return value matters, I suggest to use ...
>> `---------------------------------------------
>
> Exactly. I use `when' if side-effects are involved or if the code is too
> long and doesn't fit nicely in a `and' branch.
>
> Of course, this is nitpicking. There's nothing wrong with `when'
> variant.

I think I get it now. Thanks a lot.

Alan

Re: org-review-schedule

[Alan Schmitt]

On 2014-04-25 10:02, Nicolas Goaziou <n.goaziou@gmail.com> writes:

> Alan Schmitt <alan.schmitt@polytechnique.org> writes:
>
>> I guess I should have asked: who decides what goes in contrib?
>
> The Org maintainer. Another option is to turn it into an ELPA package.

I need to learn how to do this. In the meantime, I've put the code on
github: https://github.com/brabalan/org-review

Alan

Re: org-review-schedule

[AW]

Am Sonntag, 27. April 2014, 10:09:35 schrieb Alan Schmitt:
> On 2014-04-25 10:02, Nicolas Goaziou <n.goaziou@gmail.com> writes:
> > Alan Schmitt <alan.schmitt@polytechnique.org> writes:
> >> I guess I should have asked: who decides what goes in contrib?
> > 
> > The Org maintainer. Another option is to turn it into an ELPA package.
> 
> I need to learn how to do this. In the meantime, I've put the code on
> github: https://github.com/brabalan/org-review
> 
> Alan

I'm really interessted in a more sophisticated review system than to put 
"REVIEW" on the list of TODO | DONE items. I guess you are having a good point 
here and your code will improve orgmode. 

But after reading your initial post and the README on github I'm not sure how 
to use your code. 

Could you complete your Example in the README, please?

1. Probably -- sorry, I'm lacking of lisp understanding -- we need to put the 
file org-review.el into a place where Emacs can find it.

2. It is probably necessary to add (require 'org-review) before the code in 
the example into the ".emacs" file, or am I wrong?

3. Usually people have something like 

#+TODO: TODO INPUT ASK MAYBE | DONE

in their org-file or an equivalent in their .emacs file. What will happen to 
such customisations? 

In short, could you elaborate for users without much understanding of lisp?

Thank you for your time and effort!

Alexander

Re: org-review-schedule

[Alan Schmitt]

Hi Alexander,

On 2014-04-28 09:20, AW <alexander.willand@t-online.de> writes:

> Am Sonntag, 27. April 2014, 10:09:35 schrieb Alan Schmitt:
>> On 2014-04-25 10:02, Nicolas Goaziou <n.goaziou@gmail.com> writes:
>> > Alan Schmitt <alan.schmitt@polytechnique.org> writes:
>> >> I guess I should have asked: who decides what goes in contrib?
>> > 
>> > The Org maintainer. Another option is to turn it into an ELPA package.
>> 
>> I need to learn how to do this. In the meantime, I've put the code on
>> github: https://github.com/brabalan/org-review
>> 
>> Alan
>
> I'm really interessted in a more sophisticated review system than to
> put "REVIEW" on the list of TODO | DONE items. I guess you are having
> a good point here and your code will improve orgmode.
>
> But after reading your initial post and the README on github I'm not
> sure how to use your code.
>
> Could you complete your Example in the README, please?

I've just expanded on the README. Please let me know if you have
additional questions.

> 1. Probably -- sorry, I'm lacking of lisp understanding -- we need to
> put the file org-review.el into a place where Emacs can find it.

Yes.

> 2. It is probably necessary to add (require 'org-review) before the
> code in the example into the ".emacs" file, or am I wrong?

Yes, it is necessary.

> 3. Usually people have something like 
>
> #+TODO: TODO INPUT ASK MAYBE | DONE
>
> in their org-file or an equivalent in their .emacs file. What will
> happen to such customisations?

Nothing. Review tracking is done using properties, not keywords.

Best,

Alan

Re: org-review-schedule

[Bastien]

Hi Alan,

Alan Schmitt <alan.schmitt@polytechnique.org> writes:

> On 2014-04-25 10:02, Nicolas Goaziou <n.goaziou@gmail.com> writes:
>
>> Alan Schmitt <alan.schmitt@polytechnique.org> writes:
>>
>>> I guess I should have asked: who decides what goes in contrib?
>>
>> The Org maintainer. Another option is to turn it into an ELPA package.
>
> I need to learn how to do this.

See http://savannah.gnu.org/git/?group=emacs

You clone the ELPA repository:

  git clone <membername>@git.sv.gnu.org:/srv/git/emacs/elpa.git

Then add your package.  elpa.git contains some documentation.

-- 
 Bastien

Re: org-review-schedule

[Bastien]

Hi Alan,

Alan Schmitt <alan.schmitt@polytechnique.org> writes:

> I need to learn how to do this. In the meantime, I've put the code on
> github: https://github.com/brabalan/org-review

Since the big secret plan to move contrib/ files to Org ELPA is not
yet to happen, and since it may take time for you to add org-review
to GNU ELPA, I simply suggest you add this file to contrib/.

It will get exposure when we announce it for the upcoming 8.3 release.

Up to you!  Thanks,

-- 
 Bastien

Re: org-review-schedule

[Alan Schmitt]

Hi Bastien,

On 2014-05-15 12:07, Bastien <bzg@gnu.org> writes:

> Hi Alan,
>
> Alan Schmitt <alan.schmitt@polytechnique.org> writes:
>
>> I need to learn how to do this. In the meantime, I've put the code on
>> github: https://github.com/brabalan/org-review
>
> Since the big secret plan to move contrib/ files to Org ELPA is not
> yet to happen, and since it may take time for you to add org-review
> to GNU ELPA, I simply suggest you add this file to contrib/.

Thank you for the opportunity. I've been getting spoiled by the use of
github (to track issues, and to have documentation at the same place
than the code). I'm wondering if there is a way to have the code both on
github and in the contrib directory. Is there any current package doing
it?

Regarding the documentation, by the way, where should one put it for
contrb packages? On worg?

> It will get exposure when we announce it for the upcoming 8.3 release.

This is an important matter to take into account indeed 😉

Thanks,

Alan

Re: org-review-schedule

[Bastien]

Hi Alan,

Alan Schmitt <alan.schmitt@polytechnique.org> writes:

> Thank you for the opportunity. I've been getting spoiled by the use of
> github (to track issues, and to have documentation at the same place
> than the code). I'm wondering if there is a way to have the code both on
> github and in the contrib directory. Is there any current package doing
> it?

No.  And that's why I'm considering the migration of contributed
packages into a new Org ELPA, where sync'ing the Org ELPA package
from another repository would be possible.  A win-win situation
for everyone.

> Regarding the documentation, by the way, where should one put it for
> contrb packages? On worg?

Yes.  You can even add documentation for stuff that is not in contrib
but available on github or on some other repository service.

>> It will get exposure when we announce it for the upcoming 8.3 release.
>
> This is an important matter to take into account indeed 😉

:)

-- 
 Bastien

Re: org-review-schedule

[Alan Schmitt]

Hi Bastien,

On 2014-05-21 14:08, Bastien <bzg@gnu.org> writes:

>> Thank you for the opportunity. I've been getting spoiled by the use of
>> github (to track issues, and to have documentation at the same place
>> than the code). I'm wondering if there is a way to have the code both on
>> github and in the contrib directory. Is there any current package doing
>> it?
>
> No.  And that's why I'm considering the migration of contributed
> packages into a new Org ELPA, where sync'ing the Org ELPA package
> from another repository would be possible.  A win-win situation
> for everyone.

I've thought about this and I will try to go the ELPA route. Even if it
means less exposure, it seems to be conceptually closer to my vision of
this package (an add-on that does not need to add to the already huge
git repository).

>> Regarding the documentation, by the way, where should one put it for
>> contrb packages? On worg?
>
> Yes.  You can even add documentation for stuff that is not in contrib
> but available on github or on some other repository service.

It makes sense. I'll add "writing a tutorial on worg" on my todo list.

Thanks,

Alan