[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [O] Adding single cell movement to org-table
From: |
Nicolas Goaziou |
Subject: |
Re: [O] Adding single cell movement to org-table |
Date: |
Thu, 03 Aug 2017 12:37:18 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
Hello,
Chris Kauffman <address@hidden> 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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [O] Adding single cell movement to org-table,
Nicolas Goaziou <=