From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Goaziou Subject: Re: Adding single cell movement to org-table Date: Thu, 03 Aug 2017 12:37:18 +0200 Message-ID: <874ltplyxt.fsf@nicolasgoaziou.fr> References: <877eytug62.fsf@nicolasgoaziou.fr> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:33449) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ddDWy-0008Gi-2K for emacs-orgmode@gnu.org; Thu, 03 Aug 2017 06:39:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ddDWv-0005O8-Ar for emacs-orgmode@gnu.org; Thu, 03 Aug 2017 06:39:04 -0400 Received: from relay3-d.mail.gandi.net ([2001:4b98:c:538::195]:35920) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ddDWv-0005LG-4l for emacs-orgmode@gnu.org; Thu, 03 Aug 2017 06:39:01 -0400 In-Reply-To: (Chris Kauffman's message of "Fri, 28 Jul 2017 23:00:20 -0400") 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: Chris Kauffman Cc: Uwe Brauer , emacs-orgmode@gnu.org Hello, Chris Kauffman writes: > Apologies for the earlier diff-blast: I did not see the advice on the > org-mode contributions page that patches generated via > git format-patch master > are preferred. Please find four patches attached which now include > modifications to ORG-NEWS, org.texi, orgguid.texi, and keybindings > suggested by Carsten: S-up, S-down, S-left, S-right in org.el (via > org-shiftup etc.). Thank you! Some comments follow. > ;;;###autoload > +(defun org-table-max-line-col () > + "Return the maximum line and column of the current table as a > +list of two numbers" > + (when (not (org-at-table-p)) > + (user-error "Not in an org-table")) > + (let ((table-end (org-table-end))) > + (save-mark-and-excursion > + (goto-char table-end) > + (org-table-previous-field) > + (list (org-table-current-line) (org-table-current-column))))) I don't think this function is necessary. There is already a similar mechanism with `org-table-current-ncol' and `org-table-current-dlines'. Besides, you only need to check if point is within the table, which is trivial: (skip-chars-backward " \t") (or (bolp) (looking-at-p "[ \t]*$")) for the row, `org-table-goto-line' returns nil if there is no such line. > +;;;###autoload > +(defun org-table-swap-cells (row1 col1 row2 col2) > + "Swap two cells indicated by the coordinates provided" Final dot missing. ROW1 COL1 ROW2 COL2 should be explained. > + (let ((content1 (org-table-get row1 col1)) > + (content2 (org-table-get row2 col2))) > + (org-table-put row1 col1 content2) > + (org-table-put row2 col2 content1) > + (org-table-align))) This function can be made internal: no need to autoload it, and rename it as `org-table--swap-cells'. Besides, it shouldn't call `org-table-align', which is out of its scope. > +;;;###autoload > +(defun org-table-move-single-cell (direction) > + "Move the current cell in a cardinal direction according to the First line should be a sentence on its own. DIRECTION should be more explicit in the docstring. > +parameter symbol: 'up 'down 'left 'right. Swaps contents of `up', `down', `left' or `right'. Also, mind the two spaces after a full stop. > +adjacent cell with current one." > + (unless (org-at-table-p) > + (error "No table at point")) > + (let ((drow 0) (dcol 0)) > + (cond ((equal direction 'up) (setq drow -1)) > + ((equal direction 'down) (setq drow +1)) > + ((equal direction 'left) (setq dcol -1)) > + ((equal direction 'right) (setq dcol +1)) > + (t (error "Not a valid direction, must be one of 'up 'down 'left 'right"))) > + (let* ((row1 (org-table-current-line)) > + (col1 (org-table-current-column)) > + (row2 (+ row1 drow)) > + (col2 (+ col1 dcol)) > + (max-row-col (org-table-max-line-col)) > + (max-row (car max-row-col)) > + (max-col (cadr max-row-col))) > + (when (or (< col1 1) (< col2 1) (> col2 max-col) (< row2 1) (> row2 max-row)) > + (user-error "Cannot move cell further")) > + (org-table-swap-cells row1 col1 row2 col2) > + (org-table-goto-line row2) > + (org-table-goto-column col2)))) This should be an internal function: `org-table--move-single-cell', and not autoloaded. As an internal function, there is no need to check for `org-at-table-p'. It's the responsibility of the callers to do so. Also, `org-table-check-inside-data-field' is more appropriate here. > +;;;###autoload > +(defun org-table-move-single-cell-up () > + "Move a single cell up in a table; swap with anything in target cell" Missing final full stop. > + (interactive) > + (org-table-move-single-cell 'up)) Per above, I suggest adding (org-table-check-inside-data-field) after the (interactive). The same goes for the other functions. Could you send an updated patch? Regards, -- Nicolas Goaziou 0x80A93738