From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Goaziou Subject: Re: [patch] Improved block insertion Date: Sun, 08 Apr 2018 09:55:55 +0200 Message-ID: <87woxi2ktw.fsf@nicolasgoaziou.fr> References: <87muyeg7t5.fsf@gmx.us> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:42803) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1f55pW-00081D-Ov for emacs-orgmode@gnu.org; Sun, 08 Apr 2018 04:37:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1f55pS-0001dK-Qj for emacs-orgmode@gnu.org; Sun, 08 Apr 2018 04:37:42 -0400 Received: from relay5-d.mail.gandi.net ([217.70.183.197]:49119) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1f55pS-0001bd-JH for emacs-orgmode@gnu.org; Sun, 08 Apr 2018 04:37:38 -0400 In-Reply-To: <87muyeg7t5.fsf@gmx.us> (rasmus@gmx.us's message of "Sat, 07 Apr 2018 21:01:10 +0200") List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: "Emacs-orgmode" To: Rasmus Cc: emacs-orgmode@gnu.org Hello, Rasmus writes: > These patches improve the block insertion mechanisms using both the > keyboard binding and org-tempo. Thank you. Some quick comments follow. > Patch 6 changes the key binding of blocks to "C-c C-," as discussed in > December 2017. Let me know if this key is OK and if the old key should > still be kept. I don't think the old key-binding should be kept. > +(defun org--mks-read-key (allowed-keys prompt) > + "Read a key and ensure it is a member of ALLOWED-KEYS. > + > +Tab, space and RET are treated equivalently." Nitpick: No need for a blank line in the docstring. Also: TAB, SPC and RET are ... > + (let* ((char (read-char-exclusive prompt)) > + (key (char-to-string > + (cond ((memq char '(?\s ?\t ?\r)) ?\t) > + (t char))))) Suggestion: (key (pcase (read-char-exclusive prompt) ((or ?\s ?\t ?\r) ?\t) (char char))) > + (let ((menu-choice (org--insert-structure-template-mks))) > + (if (equal (nth 0 menu-choice) "\t") > + (read-string "Structure type: ") > + (nth 1 menu-choice))))) (pcase (org--insert-structure-template-mks) (`("\t" . ,_) (read-string "Structure type: ")) (`(,_ ,choice . ,_) choice)) > (let* ((region? (use-region-p)) > (s (if region? (region-beginning) (point))) > (e (copy-marker (if region? (region-end) (point)) t)) > @@ -13568,7 +13652,7 @@ headlines matching this string." > ;; compile tags for current headline > (setq tags-list > (if org-use-tag-inheritance > - (apply 'append (mapcar 'cdr (reverse tags-alist))) > + (apply #'append (mapcar 'cdr (reverse tags-alist))) Nitpick: (mapcar #'cdr ...) > +(defun org-tempo--keys () > + (mapcar (lambda (pair) (format "<%s" (car pair))) > + (append org-structure-template-alist > + org-tempo-keywords-alist))) Missing docstring. > +(defun org-tempo--update-maybe () > + "Test if new tags have been added." I think you should clarify the docstring. It doesn't only test if new tags have been added, but it also add templates. Regards, -- Nicolas Goaziou