From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Goaziou Subject: Re: [PATCH] org-table-beginning/end-of-field Date: Mon, 08 Sep 2014 17:30:16 +0200 Message-ID: <87wq9e9lkn.fsf@nicolasgoaziou.fr> References: <87mwaaekp8.fsf@sophokles.streitblatt.de> <871trmbe92.fsf@nicolasgoaziou.fr> <87egvme27x.fsf_-_@sophokles.streitblatt.de> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:39060) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XR0sv-0004CA-Rn for emacs-orgmode@gnu.org; Mon, 08 Sep 2014 11:29:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XR0sn-0001Ch-PO for emacs-orgmode@gnu.org; Mon, 08 Sep 2014 11:29:41 -0400 Received: from relay6-d.mail.gandi.net ([2001:4b98:c:538::198]:32911) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XR0sn-0001Ad-Fn for emacs-orgmode@gnu.org; Mon, 08 Sep 2014 11:29:33 -0400 In-Reply-To: <87egvme27x.fsf_-_@sophokles.streitblatt.de> (Florian Beck's message of "Mon, 08 Sep 2014 14:17:06 +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-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: Florian Beck Cc: emacs-orgmode@gnu.org Florian Beck writes: > How about this? Nice. Some comments follow. > > From 4fb2bbff2238d15ae7c896e0eb268b74ea4e56dc Mon Sep 17 00:00:00 2001 > From: Florian Beck > Date: Mon, 8 Sep 2014 14:08:56 +0200 > Subject: [PATCH] org-table: fix arguments of `org-table-beginning-of-field' "fix" needs to be capitalized. > and `org-table-end-of-field'. Also fix docstring and cleanup code. You need a shorter summary, e.g., org-table: Fix org-table/beginning/end/-of-field. The second sentence should go at the end of the commit message. Also, there's no full stop on summary lines. > * lisp/org-table.el (org-table--border-of-field): new function "new" needs to be capitalized. Also there's a missing full stop. > (org-table-beginning-of-field): call it > (org-table-end-of-field): call it (org-table-beginning-of-field, org-table-end-of-field): Call it. > +(defun org-table--border-of-field (n move-fn element cmp-fn) A docstring would be nice, even for a helper function. > (let ((pos (point))) > (while (> n 1) > (setq n (1- n)) > - (org-table-previous-field)) > - (if (not (re-search-backward "|" (point-at-bol 0) t)) > - (user-error "No more table fields before the current") > - (goto-char (match-end 0)) > - (and (looking-at " ") (forward-char 1))) > - (if (>= (point) pos) (org-table-beginning-of-field 2)))) > + (funcall move-fn)) While you're at it, I suggest (dotimes (_ (1- n)) (funcall move-fn)) instead of (while (> n 1) ...) Also, note that you can avoid requiring MOVE-FN, ELEMENT and CMP-FN if you decide that (< n 0) => (#'org-table-next-field :contents-end #'<=) (> n 0) => (#'org-table-previous-fierd :contents-begin #'>=) Up to you. > + (goto-char (org-element-property element (org-element-context))) You need to be more careful here. You might be outside of a table, or within an object contained in a cell, e.g., | some *bold* object | where (org-element-context) on bold will return a `bold' type object with different :contents-begin and :contents-end values. IOW, you need to let-bind (org-element-context) and find the first `table', `table-row' or `table-cell' object/element among it and its parents. Then - if no such ancestor is found: return an error (not at a table) - if `table' is found but point is not within [:contents-begin :contents-end[ interval, return an error (not inside the table) - if `table' or `table-row' is found, you need to apply org-table/previous/next/-field once (and diminish N by one) to make sure point will be left on a regular cell, if possible. > + (if (and (> n 0) (funcall cmp-fn (point) pos)) > + (org-table--border-of-field 2 move-fn element cmp-fn)))) I don't think (> n 0) is necessary, is it? Also, I suggest to use `when' (or `and' if return value matters) over one-armed `if'. > -(defun org-table-end-of-field (&optional n) > +(defun org-table-beginning-of-field (&optional n) > "Move to the beginning of the current table field. > -If already at or before the beginning, move to the beginning of the > -previous field. > +If already at or before the beginning and N is not 0, move to the > +beginning of the previous table field. > With numeric argument N, move N-1 fields backward first." I wouldn't start a new line here. Also don't forget double spaces: If already at or before the beginning and N is not 0, move to the beginning of the previous table field. With numeric argument N, move N-1 fields backward first. > + (org-table--border-of-field (or n 1) > + 'org-table-previous-field > + :contents-begin '>=)) Nitpick: #'org-table-previous-field and #'>=. > +(defun org-table-end-of-field (&optional n) > + "Move to the end of the current table field. > +If already at or after the end and N is not 0, move to the end of the > +next field. > +With numeric argument N, move N-1 fields forward first." > + (interactive "p") > + (org-table--border-of-field (or n 1) > + 'org-table-next-field > + :contents-end '<=)) Ditto. Thank you for taking care of this. There are bonus points if you can write tests along with this change. Regards, -- Nicolas Goaziou