From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Goaziou Subject: Re: [PATCH] org-agenda: Add 'none setting for org-agenda-overriding-header Date: Sun, 20 Aug 2017 10:25:38 +0200 Message-ID: <87wp5yir25.fsf@nicolasgoaziou.fr> References: <87k22416un.fsf@alphapapa.net> <87bmnep8yz.fsf@nicolasgoaziou.fr> <8760dmynkk.fsf@alphapapa.net> <871so9nt04.fsf@nicolasgoaziou.fr> <87pobrc5vp.fsf@alphapapa.net> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:38065) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1djLYH-0002iv-D2 for emacs-orgmode@gnu.org; Sun, 20 Aug 2017 04:25:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1djLYE-0000Mr-6J for emacs-orgmode@gnu.org; Sun, 20 Aug 2017 04:25:45 -0400 Received: from relay4-d.mail.gandi.net ([217.70.183.196]:46008) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1djLYD-0000M8-VW for emacs-orgmode@gnu.org; Sun, 20 Aug 2017 04:25:42 -0400 In-Reply-To: <87pobrc5vp.fsf@alphapapa.net> (Adam Porter's message of "Sat, 19 Aug 2017 21:47:22 -0500") 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: Adam Porter Cc: emacs-orgmode@gnu.org Hello, Adam Porter writes: > Here is the new patch. Thank you. > I took the liberty of using a macro to replace the code that was > duplicated in the four agenda functions. Please let me know if you > would like any further changes. Some comments follow. > From 203bc583da0c482ab7092623383fe47c2d729420 Mon Sep 17 00:00:00 2001 > From: Adam Porter > Date: Sat, 19 Aug 2017 21:26:12 -0500 > Subject: [PATCH] org-agenda: Refactor org-agenda-overriding-header code > > Replace org-agenda-overriding-header tests in these four functions with > calls to a macro, eliminating the duplicate code. Also, disable the > header when the variable is set to the empty string. Nitpick: the paragraph above usually comes after the list of changes. > * lisp/org-agenda.el (org-agenda--insert-overriding-header): Add macro. > (org-agenda-list): Use macro. > (org-search-view): Use macro. > (org-todo-list): Use macro. > (org-tags-view): Use macro. Nitpick: you can only write "Use macro" once, on the last line. > +(cl-defmacro org-agenda--insert-overriding-header (&key default) Why `cl-defmacro'? Usually, `defmacro' is enough. > + "Insert header into agenda view depending on value of `org-agenda-overriding-header'. > +If the empty string, don't insert a header. If any other string, > +insert it as a header. If nil, insert DEFAULT, which should > +evaluate to a string." > + (declare (debug (&key form))) > + `(pcase org-agenda-overriding-header > + ("" nil) ; Don't insert a header if set to empty string > + ;; Insert user-specified string > + ((pred stringp) (insert > + (org-add-props (copy-sequence org-agenda-overriding-header) > + nil 'face 'org-agenda-structure) > + "\n")) While we're at it, `org-add-props' + `copy-sequence' = `propertize' > + ;; When nil, make string automatically and insert it > + ((pred null) (insert ,default)))) I suggest to use ,@default (and, obviously (&rest default) in the arguments) so we are not limited to one S-exp. > + (org-agenda--insert-overriding-header > + :default (concat (org-agenda-span-name span) > + "-agenda" > + (if (< (- d2 d1) 350) > + (if (= w1 w2) > + (format " (W%02d)" w1) > + (format " (W%02d-W%02d)" w1 w2)) > + "") If you're ready for further refactoring, the nested `if' above could be turned into a nicer `cond'. > + (let ((n 0) s) > + (mapc (lambda (x) > + (setq s (format "(%d)%s" (setq n (1+ n)) x)) > + (if (> (+ (current-column) (string-width s) 1) (frame-width)) > + (insert "\n ")) > + (insert " " s)) > + kwds)) > + (insert "\n")) Same here: `mapc' + `lambda' = `dolist' to avoid funcall overhead. `s' could be let-bound too. All in all, the only requested change is `cl-defmacro' -> `defmacro'. This is no blocker if you don't want/have time to do the refactoring. Regards, -- Nicolas Goaziou