Xiaoduan Chang <[email protected]> writes: > I spent some time on the org-mode project of FSF40 hackason. I know it > is already too late to be considered officially taking part in the > event, I still want to share my effort on it.
Thanks for sharing! Late or not, the goal of FSF40 hackathon is to bring more people to free software contributions. > And, wait, before you > close, there is another helpful (maybe) advice for > `org-archive-subtree'. I discovered that its doc string says: > > > ... The tree will be moved to that location, the subtree heading be > marked DONE, and the current time will be added. > When I was testing with `org-archive-subtree', I found that it did not > mark the archived subtree as DONE, which was confusing me. Then when I > read the code, I found that this behavior actually depends on the value > of the variable `org-archive-mark-done'. I guess this variable was > introduced after the doc string was written? Anyway, without checking > the code and having knowledge of this variable, it may confuse users if > they only read the doc string of the function and find that it does not > behave as said. Your guess is plausible, although org-archive pre-dates git history records. Do you want to fix the docstring yourself and send a patch? > Okay, come back to the FSF40 hackason project. I do have a minimal and > naive `org-unarchive-subtree' implementation. I am really new to Emacs > Lisp hacking and so I may make really naive assumptions, but at least I > have tested the code and at least in my case it does work as expected. > So, I am just sharing it and hope that some one may point out some of my > silly mistakes and if I have violated some of the coding conventions. (I > did read the documentation about Emacs Lisp coding conventions, but I > can not guarantee that I have remembered them all.) Please be gentle > about my mistakes though :) No problem. I looked at your code and I note that its logic is roughly similar to what Thomas shared in https://list.orgmode.org/[email protected]/ (https://git.sr.ht/~taingram/org-unarchive/tree/master/item/org-unarchive.el#L19) You may consider joining him as he plans to continue working on the project: >> I'll finish this up in the next couple weeks if anyone wants to collaborate >> on it further I will still provide more specific comments inline for instruction purposes. > +;;;###autoload > +(defun org-unarchive-subtree () > + "Try moving the subtree at point back to where it was archived from. > +If the original file is missing, the process is aborted. If the original > +heading is missing, then it is inserted as a top level subtree." The docstring look good. > + (org-copy-subtree 1 nil t) Note how `org-archive-subtree' does ;; We first only copy, in case something goes wrong ;; we need to protect `this-command', to avoid kill-region sets it, ;; which would lead to duplication of subtrees (let (this-command) (org-copy-subtree 1 nil t)) Probably a good idea to follow the same logic. > + (save-excursion > + (switch-to-buffer (find-file-noselect file)) `switch-to-buffer' is not advised to be used in Elisp programs. If you look into the docstring, you will see Signature (switch-to-buffer BUFFER-OR-NAME &optional NORECORD FORCE-SAME-WINDOW) Documentation Display buffer BUFFER-OR-NAME in the selected window. WARNING: This is NOT the way to work on another buffer temporarily within a Lisp program! Use set-buffer instead. That avoids messing with the window-buffer correspondences. Even better than `set-buffer' is `with-current-buffer'. > + (let* ((headings (mapcar (lambda (l) > + (list (mapconcat 'identity (car l) "/") > + (car (cdr l)))) Can also simply say (nth 1 l) > + (org-map-entries (lambda () (list > (org-get-outline-path t) (point)))))) > + (old-parent-heading (let ((-compare-fn (lambda (s l) > + (equal s (car l))))) > + (-contains? headings olpath)))) This can work, but has several issues: 1. You are using dash.el, which is a third-party library. In Org mode, we generally limit ourselves to built-in Emacs libraries and do not rely on third-party packages. 2. I slightly simpler (but similar) approach would be using `cl-find' with :test argument where you can specify the comparator as lambda 3. An even simpler approach would be using `org-find-olp' :) 4. Note that there is an edge case when headline itself contains "/" > + (if old-parent-heading > + (let ((pos (car (cdr (car old-parent-heading))))) This is awkward. car->cdr->car is hard to comprehend in practice. When you need to store complex data, a much simpler approach is using plists: (:olp <your outline path> :pos <position>). Querying plists with reasonable key names is then much easier to read in the code. > + (goto-char pos) > + (org-paste-subtree '(16))) > + (message "WARNING: Can not locate the original parent heading, > unarchiving as a top level heading!") This warning clearly indicates that you yourself think that unarchiving to a top level heading will likely catch users by surprise. You can do two more sensible approaches instead: 1. Actually query user with `y-or-n-p' 2. Allow users to "force" unarchiving by passing a prefix argument to the whole `org-unarchive-subtree' function. > I noticed when I was compiling the file that there is a warning: > > In org-unarchive-subtree: > org-archive.el:681:41: Warning: Unused lexical variable ‘-compare-fn’ > > Though it is not accessed directly, it is used by `-contains?', then how > do I deal with this warning? Also, it seems that `-contains?' is > implemented by dash.el. Should I require it in this function? Or should > I declare it has already been defined? Generally speaking, how should I > tell if a function is already defined so that I need only declare it > rather than requiring the package? Adding (require 'dash) at the top level will get rid of the warning. dash.el defines `-compare-fn' variable. You can also add (require 'dash) inside function body, but then you may additionally need to declare the variable and function as (defvar -compare-fn) (declare-function -contains? "dash" (list element)) That said, as I stated above, we generally do not use third-party packages and prefer built-in libraries. > Also, I am a little bit confused about the commit messages and ChangeLog > entries part in the contributing documentation. Are these messages part > of the patch file or they should only present in this e-mail? When you have a separate patch file, the commit message will be a part of it. I added relevant clarification to the "org-contribute" page. https://git.sr.ht/~bzg/worg/commit/968769ff > Anyway, I have my git commit message following the requirement > presented in the documentation. And I append it as follows: > > lisp/org-archive.el: implement org-unarchive-subtree > > * org-archive.el (org-unarchive-subtree): implement > org-unarchive-subtree, which tries to restore archived headings to its > original location. > > Do I miss anything to contribute to org-mode? Thanks for pointing out. Mostly fine, with two minor comments: 1. Usually, imperative or declarative style is preferred to avoid repetitions * org-archive.el (org-unarchive-subtree): New command that tries to restore archived heading to its original location. 2. ": " is followed by a new sentence that should start from capital letter. -- Ihor Radchenko // yantar92, Org mode maintainer, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92>
