Re: Proposal: 'executable' org-capture-templaes

[Arthur Miller]

Ihor Radchenko <yantar92@gmail.com> writes:

> Arthur Miller <arthur.miller@live.com> writes:
>
>> I have reworked a bit org-capture, but I haven't yet worked on org-agenda
>> restrictions and other details.
>
> I do not think that you need to prioritize re-creating
> org-capture/org-agenda/etc The main point is making sure that org-select
> provides the required functionality.

That is just my way to test if it provides features needed, and how they
would reflect on real use-case such as org-capture and org-agenda. It is
just for demo purpose so to say.

> I'd prefer to first finalize the API and get a cleaner code of
> org-select itself.
 
Yes, that is my goal too. I just thought it was illustrative in context
of those two.

>> (define-minor-mode org-select-mode ""
>>   :interactive nil :global nil)
>
> Why don't you just make it a major mode derived from special-mode? It
> will make more sense considering that you are setting special-mode,
> keymap, etc.

I don't remember any more :). The first version I made was derived from
special mode, but than I switched to minor mode. I don't remember why
atm, to be honset, but I don't think there is anything prohibitive to
derive it from special mode either.

>> (defun osl--prop (property list)
>>   "Return value of PROPERTY from irregular plist LIST."
>>   (cadr (member property list)))
>
> FYI, there is plist-get

Yes I know, I have been using it. However this one is a bit
different. plist-get works on regular lists, this one works on
irregular. This one just match key-value in any list, or to be correct,
it matches key and return next element as the value.

I should have documented, but I meant to document them later on, this
is just for my testing and demoing.

>> (defun osl--init ()
>>   (buffer-local-value 'osl--init (current-buffer)))
>
> This is no different than just saying osl--init.
>
>> (defun org-select-abort ()
>>   (interactive)
>>   (org-select-quit "Aborted"))
>
> Please make sure that all the functions and variables have docstrings.
> This is not just a boring convention, it really helps when you come back
> to the code in future and when other people are reading your code.

Yepp. As said, this was just while I am testing and demoing. Since I
tend to change lots in the code, I am finding myself constantly typing
and erasing stuff, but I guess I should have been more clear when
sending in code to others. I also forgott to make patch and sent in
everything, wasn't really meaning :):

>> (defun osl--longest-line ()
>>   "Return the length of the longest line in current buffer."
>>   (let ((n 1) (L 0) (e 0) (E (point-max)) l)
>>     (while (< e E)
>>       (setq e (line-end-position n)
>>             l (- e (line-beginning-position n))
>>             n (1+ n))
>>       (if (> l L) (setq L l)))
>>     L))
>
> Please do not use single-char variable names for non-trivial variables.
> It is always better to provide self-explanatory names. It is not a
> programming context. We are targeting better readability, not fast
> typing.

Ah, that one is special :). e = end, l = line length L = longest
length. I espect myself to rework that one, but yes normally I use
self-docummented code. Sure np, I'll try to not send in short-named ones.

>>       (dolist (menu menus)
>>         (cond ((symbolp menu) (setq menu (eval menu)))
>>               ((symbolp (car menu)) (setq menu (eval (car menu)))))
>>         (let ((handler (osl--prop :org-select-handler menu)))
>>           (when handler
>>             (setq menu (delete :org-select-handler (delete handler menu))))
>
> Destructive modifications of arguments is a bad idea. I expect future
> bugs in such code. Please avoid this approach.

Ok. I can rework it by copying the list, but I am not sure if it adds
much of the value since the original one is not used after this
point. But if it is believed to lead to bugs, I can of course change it,
its not a problem.

>> ;; we send in the entire menu so we can return next piece in chain,
>> ;; but *the* entry we work with is just the very first one (car menu)
>> (defun osl--do-entry (menu handler)
>>   "Display a single entry in the buffer."
>
> AFAIU, the comment on top belongs to the docstring. At least the part
> talking about the return value. If the function is expected to return
> something, it should be documented. Otherwise, I expect bugs in future.

Ok.

>> (defun org-select-run (entry &optional _org-select-buffer)
>>   "Try to execute form found in ENTRY if any leave ORG-SELECT-BUFFER live.
>>
>> This handler provides an easy way to use the framework for the simple
>> use-cases for multiple choices. It relies on the user to press built-in 
>> choice
>> `q' or `C-g' to exit the menu."
>
> Please, do not use key bindings verbatim in docstring. Prefer commands.
> Docstrings do have special format for auto-detecting command bindings.
> See D.6 Tips for Documentation Strings section of Elisp manual.

Allright, I didn't know about key bindings format in docs. I'll check on
it, thanks.

>>       ;; given a list of menus, display one menu at a time
>>       (dolist (menu menus)
>>      (cond ((symbolp menu) (setq menu (eval menu)))
>>            ((symbolp (car menu)) (setq menu (eval (car menu)))))
>
> Please avoid eval when possible. It can behave not nicely in
> lexical/dynamic scope.
>
>> Each menu can be followed by some properties in form of a keu-value
>> pair. The
>                                                             ^key
>> entire menu or entry does not need to be a regular plist. Following keys are
>> recognized:
>>
>> :org-select-pin     Pin this menu in org-select buffer. If group nodes are 
>> used,
>>                     when this option is `t', keep this menu visible even when
>>                     descending into a submenu. ;; FIXME Not implemented yet.
>> :org-select-handler Use this function to handle this particular menu or
>>                     entry. When none is specified, org-select uses
>>                     `org-select-run-once' to hande the menu. Entry handler
>>                     takes precedence over menu handler.
>
> This is confusing. What do you mean by "does not need to be a regular
> plist"? What do you mean by "menu can be followed"? Do you imply that
> MENUS may not be a list of MENU lists, but can also contain :key value
> pairs?
Yepp. That is also the reason for osl--prop above, and why am I using
"destructive approach" when I remove the handler from the list before
rendering it.

Consider simple case of (key label form) that can be executed directly
as in tests at the end. I can than add a key-value pair like, and get
the value with osl--prop. Otherwise (key label form) would need an extra
keyword or some other mean: (key label :form form)?

> In general, I'd prefer format similar to
> https://github.com/progfolio/doct/
> or at least similar to org-capture-templates
> The global ARGS in org-select could be defined using cl-defun style. See
> 2.1 Argument Lists section of CL manual.

Ok. I am not familiar with doct, but I did try to keep it similar to
org-capture-templates. Or at least it must start with key and label:

(key label ... )

rest after label is user data. This entry will be passed back to user
callback if user specify a callback. The special case is a simplistic
case where client code does not have any data but will just like to have
a lisp form executed when user chooses a key. That one is (key label
form). I think that will be the most often use-case. That would provide
for really simple way to create menus, so I would like to make the most
often use-case the most simple to use too. In more general case, as in
org-capture for example, I exect client code to pass in a handler and
parse its user data itself. If I relate to Max last mail about C structs
and void* to user data, that is pretty much that void* user
gets. Org-capture define its own "template language", some other project
could define their own, etc. The only thing this library cares about is
the first two elements so it can draw stuff in buffer and create
mode-map to execute commands.

Thank you for the input and sorry for sending in the entire program
instead of just patch, I tottally forgot it and remembered first long
after I sent the mail.

I'll definitely think about all the issues you bring up, and
hear from me when I have reworked it a bit more.

best regards
/a