[PATCH] Re: [BUG] org-colview tests fail in Emacs 29 because of wrong org-colview's assumptions about current-column [9.5.4 (release_9.5.4-626-g45f9d8.dirty @ /home/yantar92/.emacs.d/straight/build/org/)]

[Ihor Radchenko <yantar92@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

Ihor Radchenko <yantar92@gmail.com> writes:

> On Emacs 29, make test will fail on both bugfix and main:
>
> 5 unexpected results:
>    FAILED  test-org-colview/columns-move-left
>    FAILED  test-org-colview/columns-move-right
>    FAILED  test-org-colview/columns-new
>    FAILED  test-org-colview/columns-next-allowed-value
>    FAILED  test-org-colview/columns-update

I am attaching a tentative fix.

Also, note that `current-column' is used, for example, in org-element
list parser. If users use some kind of weird font-lock that replaces
indentation in Org lists, our parser may become faulty. Similarly,
`current-column' is being used in few more places across Org.

I am not yet fixing such possible issues, but it would be helpful if
someone comments on the considered possibilities.

Best,
Ihor


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-colview-Do-not-rely-on-current-column-ignoring-d.patch --]
[-- Type: text/x-patch, Size: 6961 bytes --]

From 8f56cf22ff1727542a237a2ed39d48eca1479414 Mon Sep 17 00:00:00 2001
Message-Id: <8f56cf22ff1727542a237a2ed39d48eca1479414.1659154264.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Sat, 30 Jul 2022 12:07:14 +0800
Subject: [PATCH] org-colview: Do not rely on `current-column' ignoring display
 properties

* lisp/org-macs.el (org-current-text-column): New macro calculating
current column without accounting display text properties.

* lisp/org-colview.el (org-columns-check-computed):
(org-columns-next-allowed-value):
(org-columns-new):
(org-columns-delete):
(org-columns-edit-attributes):
(org-columns-widen):
(org-columns-move-right):
(org-columns-move-left):
(org-columns-update): Use the new macro when calculating point
position in the column view table overlay.  Do _not_ use the new
macro when we want to get the visual column position of the point.

Fixes "test-org-colview/" failures on Emacs 29 after Emacs commit
4243747b1b8c3b7e3463822804b32e83febe2878:

;; Fix 'current-column' in the presence of display strings

;; * src/indent.c (check_display_width): Support calculation of width
;; of 'display' properties whose values are strings.  This fixes the
;; value returned by 'current-column' when display strings are
;; present between BOL and point.  (Bug#53795)

See https://orgmode.org/list/CACnOyijQc7BDDtrYQb+=VoGWkpWAyMu7O4qsvGpsU6SCgwiM8Q@mail.gmail.com
---
 lisp/org-colview.el | 25 +++++++++++++------------
 lisp/org-macs.el    |  5 +++++
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/lisp/org-colview.el b/lisp/org-colview.el
index 13643101b..2829678f5 100644
--- a/lisp/org-colview.el
+++ b/lisp/org-colview.el
@@ -557,7 +557,7 @@ (defun org-columns-quit ()
 
 (defun org-columns-check-computed ()
   "Throw an error if current column value is computed."
-  (let ((spec (nth (current-column) org-columns-current-fmt-compiled)))
+  (let ((spec (nth (org-current-text-column) org-columns-current-fmt-compiled)))
     (and
      (nth 3 spec)
      (assoc spec (get-text-property (line-beginning-position) 'org-summaries))
@@ -713,7 +713,8 @@ (defun org-columns-next-allowed-value (&optional previous nth)
 an integer, select that value."
   (interactive)
   (org-columns-check-computed)
-  (let* ((column (current-column))
+  (let* ((column (org-current-text-column))
+         (visible-column (current-column))
 	 (key (get-char-property (point) 'org-columns-key))
 	 (value (get-char-property (point) 'org-columns-value))
 	 (pom (or (get-text-property (line-beginning-position) 'org-hd-marker)
@@ -763,7 +764,7 @@ (defun org-columns-next-allowed-value (&optional previous nth)
 	;; the right place on the current line.
 	(let ((org-columns-inhibit-recalculation)) (org-columns-redo))
 	(org-columns-update key)
-	(org-move-to-column column))))))
+	(org-move-to-column visible-column))))))
 
 (defun org-colview-construct-allowed-dates (s)
   "Construct a list of three dates around the date in S.
@@ -925,14 +926,14 @@ (defun org-columns-new (&optional spec &rest attributes)
     (if spec
 	(progn (setcar spec (car new))
 	       (setcdr spec (cdr new)))
-      (push new (nthcdr (current-column) org-columns-current-fmt-compiled)))
+      (push new (nthcdr (org-current-text-column) org-columns-current-fmt-compiled)))
     (org-columns-store-format)
     (org-columns-redo)))
 
 (defun org-columns-delete ()
   "Delete the column at point from columns view."
   (interactive)
-  (let ((spec (nth (current-column) org-columns-current-fmt-compiled)))
+  (let ((spec (nth (org-current-text-column) org-columns-current-fmt-compiled)))
     (when (y-or-n-p (format "Are you sure you want to remove column %S? "
 			    (nth 1 spec)))
       (setq org-columns-current-fmt-compiled
@@ -942,18 +943,18 @@ (defun org-columns-delete ()
       ;; updating it may prove counter-intuitive.  See comments in
       ;; `org-columns-move-right' for details.
       (let ((org-columns-inhibit-recalculation t)) (org-columns-redo))
-      (when (>= (current-column) (length org-columns-current-fmt-compiled))
+      (when (>= (org-current-text-column) (length org-columns-current-fmt-compiled))
 	(backward-char)))))
 
 (defun org-columns-edit-attributes ()
   "Edit the attributes of the current column."
   (interactive)
-  (org-columns-new (nth (current-column) org-columns-current-fmt-compiled)))
+  (org-columns-new (nth (org-current-text-column) org-columns-current-fmt-compiled)))
 
 (defun org-columns-widen (arg)
   "Make the column wider by ARG characters."
   (interactive "p")
-  (let* ((n (current-column))
+  (let* ((n (org-current-text-column))
 	 (entry (nth n org-columns-current-fmt-compiled))
 	 (width (aref org-columns-current-maxwidths n)))
     (setq width (max 1 (+ width arg)))
@@ -969,7 +970,7 @@ (defun org-columns-narrow (arg)
 (defun org-columns-move-right ()
   "Swap this column with the one to the right."
   (interactive)
-  (let* ((n (current-column))
+  (let* ((n (org-current-text-column))
 	 (cell (nthcdr n org-columns-current-fmt-compiled))
 	 e)
     (when (>= n (1- (length org-columns-current-fmt-compiled)))
@@ -993,7 +994,7 @@ (defun org-columns-move-right ()
 (defun org-columns-move-left ()
   "Swap this column with the one to the left."
   (interactive)
-  (let* ((n (current-column)))
+  (let* ((n (org-current-text-column)))
     (when (= n 0)
       (error "Cannot shift this column further to the left"))
     (backward-char 1)
@@ -1039,7 +1040,7 @@ (defun org-columns-update (property)
        (let ((key (overlay-get ov 'org-columns-key)))
 	 (when (and key (equal key p) (overlay-start ov))
 	   (goto-char (overlay-start ov))
-	   (let* ((spec (nth (current-column) org-columns-current-fmt-compiled))
+	   (let* ((spec (nth (org-current-text-column) org-columns-current-fmt-compiled))
 		  (value
 		   (or (cdr (assoc spec
 				   (get-text-property (line-beginning-position)
@@ -1049,7 +1050,7 @@ (defun org-columns-update (property)
 	       (let ((displayed (org-columns--displayed-value spec value))
 		     (format (overlay-get ov 'org-columns-format))
 		     (width
-		      (aref org-columns-current-maxwidths (current-column))))
+		      (aref org-columns-current-maxwidths (org-current-text-column))))
 		 (overlay-put ov 'org-columns-value value)
 		 (overlay-put ov 'org-columns-value-modified displayed)
 		 (overlay-put ov
diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index 1dc0b7080..44060f831 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -1037,6 +1037,11 @@ (defun org-string-width (string &optional pixels)
               pixel-width
             (/ pixel-width symbol-width)))))))
 
+(defmacro org-current-text-column ()
+  "Like `current-column' but ignore display properties."
+  `(string-width (buffer-substring-no-properties
+                  (line-beginning-position) (point))))
+
 (defun org-not-nil (v)
   "If V not nil, and also not the string \"nil\", then return V.
 Otherwise return nil."
-- 
2.35.1