Re: Adding single cell movement to org-table

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Chris Kauffman <kauffman@cs.gmu.edu> 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