emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* Bug: org-store-link uses CUSTOM_ID instead of target point [9.4.4 (release_9.4.4 @ /usr/share/emacs/27.2/lisp/org/)]
@ 2021-05-04 19:17 Fr Ml
  2021-05-05  2:34 ` Ihor Radchenko
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Fr Ml @ 2021-05-04 19:17 UTC (permalink / raw)
  To: emacs-orgmode

[-- Attachment #1: Type: text/html, Size: 1641 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Bug: org-store-link uses CUSTOM_ID instead of target point [9.4.4 (release_9.4.4 @ /usr/share/emacs/27.2/lisp/org/)]
  2021-05-04 19:17 Bug: org-store-link uses CUSTOM_ID instead of target point [9.4.4 (release_9.4.4 @ /usr/share/emacs/27.2/lisp/org/)] Fr Ml
@ 2021-05-05  2:34 ` Ihor Radchenko
  2021-05-06 12:41 ` Bastien
  2021-05-06 20:08 ` Table alignment problem Fr Ml
  2 siblings, 0 replies; 13+ messages in thread
From: Ihor Radchenko @ 2021-05-05  2:34 UTC (permalink / raw)
  To: Fr Ml; +Cc: emacs-orgmode

Fr Ml <fr_ml@t-online.de> writes:
> If the cursor is on a <<target>> and the item has a CUSTOM_ID then a
> link to the headline (with the CUSTOM_ID) is stored - and not the link to the target.
> This behavior is not always the same.

Confirmed on master.

Recipe:

1. Create the following org file:

* test
:PROPERTIES:
:CUSTOM_ID: testasd
:END:

<<targetasd>>

2. Put point at the target
3. org-store-link
4. org-insert-link
5. The inserted link is pointing to the custom ID
[[#testasd][file:/tmp/4.org::targetasd]]

Best,
Ihor


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Bug: org-store-link uses CUSTOM_ID instead of target point [9.4.4 (release_9.4.4 @ /usr/share/emacs/27.2/lisp/org/)]
  2021-05-04 19:17 Bug: org-store-link uses CUSTOM_ID instead of target point [9.4.4 (release_9.4.4 @ /usr/share/emacs/27.2/lisp/org/)] Fr Ml
  2021-05-05  2:34 ` Ihor Radchenko
@ 2021-05-06 12:41 ` Bastien
  2021-11-06 12:51   ` Max Nikulin
  2021-05-06 20:08 ` Table alignment problem Fr Ml
  2 siblings, 1 reply; 13+ messages in thread
From: Bastien @ 2021-05-06 12:41 UTC (permalink / raw)
  To: Fr Ml; +Cc: emacs-orgmode

Hi,

Fr Ml <fr_ml@t-online.de> writes:

> I have a problem with the function org-store-link it doesn't work as
> described in the documentation:
> https://orgmode.org/manual/Handling-Links.html
> "For Org files, if there is a '<<target>>' at point, the link points
> to
> the target."

Fixed in maint, thanks a lot for reporting this and Ihor for
confirming the bug.

-- 
 Bastien


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Table alignment problem
  2021-05-04 19:17 Bug: org-store-link uses CUSTOM_ID instead of target point [9.4.4 (release_9.4.4 @ /usr/share/emacs/27.2/lisp/org/)] Fr Ml
  2021-05-05  2:34 ` Ihor Radchenko
  2021-05-06 12:41 ` Bastien
@ 2021-05-06 20:08 ` Fr Ml
  2021-05-07 14:50   ` Ihor Radchenko
  2 siblings, 1 reply; 13+ messages in thread
From: Fr Ml @ 2021-05-06 20:08 UTC (permalink / raw)
  To: emacs-orgmode

[-- Attachment #1: Type: text/html, Size: 2416 bytes --]

[-- Attachment #2: arabic_table_orgmode.png --]
[-- Type: image/png, Size: 52604 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Table alignment problem
  2021-05-06 20:08 ` Table alignment problem Fr Ml
@ 2021-05-07 14:50   ` Ihor Radchenko
  2021-05-08  6:37     ` Jeremie Juste
  2021-05-08  7:48     ` Fr Ml
  0 siblings, 2 replies; 13+ messages in thread
From: Ihor Radchenko @ 2021-05-07 14:50 UTC (permalink / raw)
  To: Fr Ml; +Cc: emacs-orgmode

Fr Ml <fr_ml@t-online.de> writes:

> Hello,
> there is an old problem with table alignment. It's mentioned here:
> https://emacs.stackexchange.com/q/30495/11498
>
> It occurs as far as I know only in 4 cases (last 4 rows):
>
> | 2 latin letters                                 | ab | (2 glyphs)            |
> | 2 arabic letters                                | من | ok (2 glyphs)         |
> | same but with 2 diacritics                      | مِنْ | also ok  (2 glyphs)   |
> | the arabich letter ا and then ل isn't a problem | ال | also ok (2 glyphs)    |
> | but ل and then ا is a problem (case1)           | لا | not ok (it's 1 glyph) |
> | also ل and then أ (case2)                       | لأ | " (it's 1 glyph)      |
> | also ل and then إ (case3)                       | لإ | " (it's 1 glyph)      |
> | also ل and then آ (case4)                       | لآ | " (it's 1 glyph)      |

Can you try with org-fold branch [1]? It implements a new way to
calculate string width.

[1] https://github.com/yantar92/org

Best,
Ihor

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Table alignment problem
  2021-05-07 14:50   ` Ihor Radchenko
@ 2021-05-08  6:37     ` Jeremie Juste
  2021-05-08  7:45       ` Fr Ml
  2021-05-08  7:48     ` Fr Ml
  1 sibling, 1 reply; 13+ messages in thread
From: Jeremie Juste @ 2021-05-08  6:37 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode, Fr Ml


Hello,

You can have some success using [1] valign-mode.

https://github.com/casouri/valign

HTH,
Jeremie


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Table alignment problem
  2021-05-08  6:37     ` Jeremie Juste
@ 2021-05-08  7:45       ` Fr Ml
  0 siblings, 0 replies; 13+ messages in thread
From: Fr Ml @ 2021-05-08  7:45 UTC (permalink / raw)
  To: Jeremie Juste, Ihor Radchenko; +Cc: emacs-orgmode

[-- Attachment #1: Type: text/html, Size: 850 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Table alignment problem
  2021-05-07 14:50   ` Ihor Radchenko
  2021-05-08  6:37     ` Jeremie Juste
@ 2021-05-08  7:48     ` Fr Ml
  2021-05-08  8:24       ` Ihor Radchenko
  1 sibling, 1 reply; 13+ messages in thread
From: Fr Ml @ 2021-05-08  7:48 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode

[-- Attachment #1: Type: text/html, Size: 2559 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Table alignment problem
  2021-05-08  7:48     ` Fr Ml
@ 2021-05-08  8:24       ` Ihor Radchenko
       [not found]         ` <5ebfdc6f-6e40-1470-4379-4a5a2b666aa7@t-online.de>
  0 siblings, 1 reply; 13+ messages in thread
From: Ihor Radchenko @ 2021-05-08  8:24 UTC (permalink / raw)
  To: Fr Ml; +Cc: emacs-orgmode

Fr Ml <fr_ml@t-online.de> writes:

> Hello,
>
> thanks for the answer. I'm not familiar with github.
> Do you mean this program branch?:
> https://github.com/yantar92/org/tree/feature/org-fold%2Bpatches

Not really. I mean the main branch in the repo (which is
feature/org-fold-universal-core). The installation instructions are in
https://github.com/yantar92/org#installation

Best,
Ihor


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] Pixel-wise table alignment
       [not found]                             ` <2c2f2204-14b1-49b2-658b-2b8b75ab86b9@t-online.de>
@ 2021-05-15  9:26                               ` Ihor Radchenko
  0 siblings, 0 replies; 13+ messages in thread
From: Ihor Radchenko @ 2021-05-15  9:26 UTC (permalink / raw)
  To: fr ml, emacs-orgmode

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

For everyone interested in mode precise table alignment,

The attached patch should allow table columns to be aligned pixel-wise.
It means, that tables should look much better with variable-pitch fonts
and unicode symbols.

I would like to get more feedback about how the patch works for
different setups. It appears to work flawlessly for me, yet Frank
reported issues (even with emacs -Q). I need more datapoints to figure
out how to fix the patch.

An example table that requires pixel alignment is attached. Please, try
to align the table and report if there are any issues. For me, the
aligned table looks like in test_screenshot2.png. It should not look
like test_screenshot.png. If it is, I would like to know the Emacs
version, OS, and results using emacs -Q, if possible.

fr ml <fr_ml@t-online.de> writes:
> I'm using Linux. I've added an org file with a screenshot of it.

Thanks!

Best,
Ihor


[-- Attachment #2: test.org --]
[-- Type: application/vnd.lotus-organizer, Size: 150 bytes --]

|  1 | 12 |
|  2 | ةة |
|  3 | وو |
|  4 | ءء |
|  5 | رر |
|  6 | زز |
|  7 | لأ |
|  8 | لا |
|  9 | لآ |
| 10 | لإ |
| 11 | ab |

[-- Attachment #3: test_screenshot2.png --]
[-- Type: image/png, Size: 6484 bytes --]

[-- Attachment #4: test_screenshot.png --]
[-- Type: image/png, Size: 6208 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0001-Align-table-columns-pixel-wise.patch --]
[-- Type: text/x-diff, Size: 12503 bytes --]

From 8743cee4d7ee266076cfcccca0a2772aac597ee0 Mon Sep 17 00:00:00 2001
Message-Id: <8743cee4d7ee266076cfcccca0a2772aac597ee0.1621069602.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Sat, 15 May 2021 16:58:54 +0800
Subject: [PATCH] Align table columns pixel-wise

* lisp/org-macs.el (org-string-width): Rewrite manual width
calculation using `window-text-pixel-size'.  Add extra optional
argument to get string width in pixels.
(org--string-from-props): Removed, as it is no longer needed for
`org-string-width'.

* lisp/org-table.el (org-table--align-field): Align field pixel-wise.
The WIDTH argument should now be in pixel units.  The alignment is
done by setting 'display text property.

(org-table-align): Align fields pixel-wise using new
`org-table--align-field' and the same ideas with 'display property.
---
 lisp/org-macs.el  | 121 ++++++++++++++++++++++------------------------
 lisp/org-table.el |  69 ++++++++++++++++++++------
 2 files changed, 112 insertions(+), 78 deletions(-)

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index cd9fd1d83..ae79ab16c 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -868,71 +868,64 @@ (defun org-split-string (string &optional separators)
 		      results		;skip trailing separator
 		    (cons (substring string i) results)))))))
 
-(defun org--string-from-props (s property beg end)
-  "Return the visible part of string S.
-Visible part is determined according to text PROPERTY, which is
-either `invisible' or `display'.  BEG and END are 0-indices
-delimiting S."
-  (let ((width 0)
-	(cursor beg))
-    (while (setq beg (text-property-not-all beg end property nil s))
-      (let* ((next (next-single-property-change beg property s end))
-	     (props (text-properties-at beg s))
-	     (spec (plist-get props property))
-	     (value
-	      (pcase property
-		(`invisible
-		 ;; If `invisible' property in PROPS means text is to
-		 ;; be invisible, return 0.  Otherwise return nil so
-		 ;; as to resume search.
-		 (and (or (eq t buffer-invisibility-spec)
-			  (assoc-string spec buffer-invisibility-spec))
-		      0))
-		(`display
-		 (pcase spec
-		   (`nil nil)
-		   (`(space . ,props)
-		    (let ((width (plist-get props :width)))
-		      (and (wholenump width) width)))
-		   (`(image . ,_)
-                    (and (fboundp 'image-size)
-                         (ceiling (car (image-size spec)))))
-		   ((pred stringp)
-		    ;; Displayed string could contain invisible parts,
-		    ;; but no nested display.
-		    (org--string-from-props spec 'invisible 0 (length spec)))
-		   (_
-		    ;; Un-handled `display' value.  Ignore it.
-		    ;; Consider the original string instead.
-		    nil)))
-		(_ (error "Unknown property: %S" property)))))
-	(when value
-	  (cl-incf width
-		   ;; When looking for `display' parts, we still need
-		   ;; to look for `invisible' property elsewhere.
-		   (+ (cond ((eq property 'display)
-			     (org--string-from-props s 'invisible cursor beg))
-			    ((= cursor beg) 0)
-			    (t (string-width (substring s cursor beg))))
-		      value))
-	  (setq cursor next))
-	(setq beg next)))
-    (+ width
-       ;; Look for `invisible' property in the last part of the
-       ;; string.  See above.
-       (cond ((eq property 'display)
-	      (org--string-from-props s 'invisible cursor end))
-	     ((= cursor end) 0)
-	     (t (string-width (substring s cursor end)))))))
-
-(defun org-string-width (string)
+(defun org-string-width (string &optional pixels)
   "Return width of STRING when displayed in the current buffer.
-Unlike `string-width', this function takes into consideration
-`invisible' and `display' text properties.  It supports the
-latter in a limited way, mostly for combinations used in Org.
-Results may be off sometimes if it cannot handle a given
-`display' value."
-  (org--string-from-props string 'display 0 (length string)))
+Return width in pixels when PIXELS is non-nil."
+  ;; Wrap/line prefix will make `window-text-pizel-size' return too
+  ;; large value including the prefix.
+  ;; Face should be removed to make sure that all the string symbols
+  ;; are using default face with constant width.  Constant char width
+  ;; is critical to get right string width from pixel width.
+  (remove-text-properties 0 (length string)
+                          '(wrap-prefix t line-prefix t face t)
+                          string)
+  (let (;; We need to remove the folds to make sure that folded table
+        ;; alignment is not messed up.
+        (current-invisibility-spec
+         (or (and (not (listp buffer-invisibility-spec))
+                  buffer-invisibility-spec)
+             (let (result)
+               (dolist (el buffer-invisibility-spec)
+                 (unless (or (memq el
+                                   '(org-fold-drawer
+                                     org-fold-block
+                                     org-fold-outline))
+                             (and (listp el)
+                                  (memq (car el)
+                                        '(org-fold-drawer
+                                          org-fold-block
+                                          org-fold-outline))))
+                   (push el result)))
+               result)))
+        (current-char-property-alias-alist char-property-alias-alist))
+    (with-temp-buffer
+      (setq-local display-line-numbers nil)
+      (setq-local buffer-invisibility-spec
+                  current-invisibility-spec)
+      (setq-local char-property-alias-alist
+                  current-char-property-alias-alist)
+      (let (pixel-width symbol-width)
+        (with-silent-modifications
+          (setf (buffer-string) string)
+          (setq pixel-width
+                (if (get-buffer-window (current-buffer))
+                    (car (window-text-pixel-size
+                          nil (line-beginning-position) (point-max)))
+                  (set-window-buffer nil (current-buffer))
+                  (car (window-text-pixel-size
+                        nil (line-beginning-position) (point-max)))))
+          (unless pixels
+            (setf (buffer-string) "a")
+            (setq symbol-width
+                  (if (get-buffer-window (current-buffer))
+                      (car (window-text-pixel-size
+                            nil (line-beginning-position) (point-max)))
+                    (set-window-buffer nil (current-buffer))
+                    (car (window-text-pixel-size
+                          nil (line-beginning-position) (point-max)))))))
+        (if pixels
+            pixel-width
+          (/ pixel-width symbol-width))))))
 
 (defun org-not-nil (v)
   "If V not nil, and also not the string \"nil\", then return V.
diff --git a/lisp/org-table.el b/lisp/org-table.el
index cc69542f9..e8b5add8a 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -4313,12 +4313,34 @@ (defun org-table--align-field (field width align)
   "Format FIELD according to column WIDTH and alignment ALIGN.
 FIELD is a string.  WIDTH is a number.  ALIGN is either \"c\",
 \"l\" or\"r\"."
-  (let* ((spaces (- width (org-string-width field)))
+  (let* ((spaces (- width (org-string-width field 'pixels)))
+         (symbol-width (org-string-width " " 'pixels))
+         (right-spaces (/ spaces symbol-width))
+         (right-pixels (- spaces (* symbol-width right-spaces)))
+         (centered-spaces (/ (/ spaces 2) symbol-width))
+         (centered-pixels (- (/ spaces 2) (* symbol-width centered-spaces)))
 	 (prefix (pcase align
 		   ("l" "")
-		   ("r" (make-string spaces ?\s))
-		   ("c" (make-string (/ spaces 2) ?\s))))
-	 (suffix (make-string (- spaces (length prefix)) ?\s)))
+		   ("r" (concat (make-string right-spaces ?\s)
+                                ;; Align to non-fixed width.
+                                (if (zerop right-pixels) ""
+                                  (propertize " "
+                                              'display
+                                              `(space . (:width (,right-pixels)))))))
+		   ("c" (concat (make-string centered-spaces ?\s)
+                                ;; Align to non-fixed width.
+                                (if (zerop centered-pixels) ""
+                                  (propertize " "
+                                              'display
+                                              `(space . (:width (,centered-pixels)))))))))
+         (suffix-spaces (/ (- spaces (org-string-width prefix 'pixel)) symbol-width))
+         (suffix-pixels (- (- spaces (org-string-width prefix 'pixel)) (* symbol-width suffix-spaces)))
+	 (suffix (concat (make-string suffix-spaces ?\s)
+                         ;; Align to non-fixed width.
+                         (if (zerop suffix-pixels) ""
+                           (propertize " "
+                                       'display
+                                       `(space . (:width (,suffix-pixels))))))))
     (concat org-table-separator-space
 	    prefix
 	    field
@@ -4342,7 +4364,8 @@ (defun org-table-align ()
              (rows (remq 'hline table))
 	     (widths nil)
 	     (alignments nil)
-	     (columns-number 1))
+	     (columns-number 1)
+             (symbol-width (org-string-width "-" 'pixels)))
 	(if (null rows)
 	    ;; Table contains only horizontal rules.  Compute the
 	    ;; number of columns anyway, and choose an arbitrary width
@@ -4352,17 +4375,17 @@ (defun org-table-align ()
 		(while (search-forward "+" end t)
 		  (cl-incf columns-number)))
 	      (setq widths (make-list columns-number 1))
-	      (setq alignments (make-list columns-number "l")))
+	      (setq alignments (make-list (* columns-number symbol-width) "l")))
 	  ;; Compute alignment and width for each column.
 	  (setq columns-number (apply #'max (mapcar #'length rows)))
 	  (dotimes (i columns-number)
-	    (let ((max-width 1)
+	    (let ((max-width symbol-width)
 		  (fixed-align? nil)
 		  (numbers 0)
 		  (non-empty 0))
 	      (dolist (row rows)
 		(let ((cell (or (nth i row) "")))
-		  (setq max-width (max max-width (org-string-width cell)))
+		  (setq max-width (max max-width (org-string-width cell 'pixels)))
 		  (cond (fixed-align? nil)
 			((equal cell "") nil)
 			((string-match "\\`<\\([lrc]\\)[0-9]*>\\'" cell)
@@ -4386,9 +4409,18 @@ (defun org-table-align ()
 	;; Build new table rows.  Only replace rows that actually
 	;; changed.
 	(let ((rule (and (memq 'hline table)
-			 (mapconcat (lambda (w) (make-string (+ 2 w) ?-))
-				    widths
-				    "+")))
+                         (mapconcat
+                          (lambda (w)
+                            (let* ((hline-dahes (+ 2 (/ w symbol-width)))
+                                   (hline-pixels (- w (* symbol-width (/ w symbol-width)))))
+                              (concat (make-string hline-dahes ?-)
+                                      ;; Align to non-fixed width.
+                                      (if (zerop hline-pixels) ""
+                                        (propertize " "
+                                                    'display
+                                                    `(:width (,hline-pixels)))))))
+			  widths
+			  "+")))
               (indent (progn (looking-at "[ \t]*|") (match-string 0))))
 	  (dolist (row table)
 	    (let ((previous (buffer-substring (point) (line-end-position)))
@@ -4408,9 +4440,18 @@ (defun org-table-align ()
 				          "|")))
 		           "|")))
 	      (if (equal new previous)
-		  (forward-line)
-		(insert new "\n")
-		(delete-region (point) (line-beginning-position 2))))))
+                  (if (equal-including-properties new previous)
+                      (forward-line)
+                    (let ((pos 0) next)
+                      (while (< pos (length new))
+                        (setq next (or (next-single-property-change pos 'display new)
+                                       (length new)))
+                        (when (get-text-property pos 'display new)
+                          (put-text-property (+ pos (point)) (+ next (point)) 'display (get-text-property pos 'display new)))
+                        (setq pos next)))
+                    (forward-line))
+	        (insert new "\n")
+	        (delete-region (point) (line-beginning-position 2))))))
 	(set-marker end nil)
 	(when org-table-overlay-coordinates (org-table-overlay-coordinates))
 	(setq org-table-may-need-update nil))))))
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: Bug: org-store-link uses CUSTOM_ID instead of target point [9.4.4 (release_9.4.4 @ /usr/share/emacs/27.2/lisp/org/)]
  2021-05-06 12:41 ` Bastien
@ 2021-11-06 12:51   ` Max Nikulin
  2022-08-14  9:58     ` Max Nikulin
  2022-09-13 13:05     ` Ihor Radchenko
  0 siblings, 2 replies; 13+ messages in thread
From: Max Nikulin @ 2021-11-06 12:51 UTC (permalink / raw)
  To: Bastien, Fr Ml; +Cc: emacs-orgmode

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

On 06/05/2021 19:41, Bastien wrote:
> Fr Ml writes:
> 
>> I have a problem with the function org-store-link it doesn't work as
>> described in the documentation:
>> https://orgmode.org/manual/Handling-Links.html
>> "For Org files, if there is a '<<target>>' at point, the link points
>> to
>> the target."
> 
> Fixed in maint, thanks a lot for reporting this and Ihor for
> confirming the bug.

Bastien, unfortunately your fix caused duplication of stored links like 
"file:~/org/file.org::#custom_id" when point is outside of <<target>>. 
Earlier #CUSTOM_ID link was stored in addition to 
"file:~/org/file.org::*Heading" search link. My suggestion is to revert 
your patch and to just reset custom-id variable when <<target>> link is 
stored. Another effect or your patch, that I consider unintentional, is 
storing 
[[file:~/org/file.org::#custom_id][file:~/org/file.org::#custom_id]] 
instead of [[file:~/org/file.org::#custom_id][Heading]]. I prefer 
"original" behavior.

Third patch is intended to avoid links inserted as 
[[target][file:~/org/file.org::target]] in the case of same file. I 
suppose, just [[target]] is better. Current variant looks unbalanced and 
misleading. Of course, you are free to skip last patch.

I am confused by `org-insert-link' behavior. It inserts links like 
[[file:~/org/file.org::#custom_id][file:~/org/file.org::#custom_id]] 
without user prompt for description but 
[[file:~/org/file.org::#custom_id][Heading]] requires to confirm 
"Heading" description explicitly. I would expect that "raw" link is 
subject to ask user for more friendly option.


[-- Attachment #2: 0001-Revert-lisp-ol.el-Fix-bug-when-storing-links.patch --]
[-- Type: text/x-patch, Size: 2114 bytes --]

From 28f1d331888ebd22d60343bb06d3b307aff9fc93 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Fri, 5 Nov 2021 19:03:59 +0700
Subject: [PATCH 1/3] Revert "lisp/ol.el: Fix bug when storing links"

This reverts commit b4b35fc92d6ea8eb2ac061b8ccf026e9b4ebfe33.

Avoid duplication of CUSTOM_ID links.
---
 lisp/ol.el | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/lisp/ol.el b/lisp/ol.el
index aa1849715..6fe50ed60 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -1613,8 +1613,9 @@ non-nil."
 
        ((and (buffer-file-name (buffer-base-buffer)) (derived-mode-p 'org-mode))
 	(org-with-limited-levels
-         (cond
-	  ;; Store a link using the target at point.
+	 (setq custom-id (org-entry-get nil "CUSTOM_ID"))
+	 (cond
+	  ;; Store a link using the target at point
 	  ((org-in-regexp "[^<]<<\\([^<>]+\\)>>[^>]" 1)
 	   (setq cpltxt
 		 (concat "file:"
@@ -1622,15 +1623,6 @@ non-nil."
 			  (buffer-file-name (buffer-base-buffer)))
 			 "::" (match-string 1))
 		 link cpltxt))
-          ;; Store a link using the CUSTOM_ID property.
-          ((setq custom-id (org-entry-get nil "CUSTOM_ID"))
-           (setq cpltxt
-		 (concat "file:"
-			 (abbreviate-file-name
-			  (buffer-file-name (buffer-base-buffer)))
-			 "::#" custom-id)
-		 link cpltxt))
-          ;; Store a link using (and perhaps creating) the ID property.
 	  ((and (featurep 'org-id)
 		(or (eq org-id-link-to-org-use-id t)
 		    (and interactive?
@@ -1639,13 +1631,14 @@ non-nil."
 				      'create-if-interactive-and-no-custom-id)
 				  (not custom-id))))
 		    (and org-id-link-to-org-use-id (org-entry-get nil "ID"))))
+	   ;; Store a link using the ID at point
 	   (setq link (condition-case nil
 			  (prog1 (org-id-store-link)
 			    (setq desc (or (plist-get org-store-link-plist
 						      :description)
 					   "")))
 			(error
-			 ;; Probably before first headline, link only to file.
+			 ;; Probably before first headline, link only to file
 			 (concat "file:"
 				 (abbreviate-file-name
 				  (buffer-file-name (buffer-base-buffer))))))))
-- 
2.25.1


[-- Attachment #3: 0002-ol.el-Skip-CUSTOM_ID-when-target-link-is-stored.patch --]
[-- Type: text/x-patch, Size: 1225 bytes --]

From 1fb5a5e071141e47d551202eeca5776185ca8461 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Fri, 5 Nov 2021 19:49:23 +0700
Subject: [PATCH 2/3] ol.el: Skip #CUSTOM_ID when <<target>> link is stored

* list/ol.el (org-store-link): Suppress storing of
"file:file.org::#custom_id" link when point is <<target>>.

CUSTOM_ID link is stored as additional option, so new chunk of code
introduced by b4b35fc92 is not necessary. It prevented adding of
"file:file.org::*Heading" link and caused duplication of
"file:file.org::#custom_id" link.

Reported-by: Fr Ml <fr_ml@t-online.de>
Link: https://orgmode.org/list/aadb23f3-c0fe-19aa-be79-50e51d16c41a@t-online.de/
---
 lisp/ol.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/ol.el b/lisp/ol.el
index 6fe50ed60..5e1f1f2d2 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -1622,7 +1622,9 @@ non-nil."
 			 (abbreviate-file-name
 			  (buffer-file-name (buffer-base-buffer)))
 			 "::" (match-string 1))
-		 link cpltxt))
+		 link cpltxt
+                 ;; Do not append #CUSTOM_ID link below.
+                 custom-id nil))
 	  ((and (featurep 'org-id)
 		(or (eq org-id-link-to-org-use-id t)
 		    (and interactive?
-- 
2.25.1


[-- Attachment #4: 0003-ol.el-Avoid-links-like-target-file-org-test.org-targ.patch --]
[-- Type: text/x-patch, Size: 1666 bytes --]

From f307c7059744b28b26f5c3b8dd4a0f563b22e586 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Sat, 6 Nov 2021 18:23:25 +0700
Subject: [PATCH 3/3] ol.el: Avoid links like
 "[[target][file:~/org/test.org::target]]"

* lisp/ol.el (org-store-link): Do not set description for "<<target>>"
links to avoid case when it is more detailed than link target.

While inserting to the same file, file part of the link target is
stripped, description is inserted without modification.  I do not think,
file path adds real value in comparison to "[[target]]" link to some
point in the same file.  A side effect is user prompt for description
since link and description are not identical any more.
---
 lisp/ol.el | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/lisp/ol.el b/lisp/ol.el
index 5e1f1f2d2..ec5427e7c 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -1617,12 +1617,17 @@ non-nil."
 	 (cond
 	  ;; Store a link using the target at point
 	  ((org-in-regexp "[^<]<<\\([^<>]+\\)>>[^>]" 1)
-	   (setq cpltxt
+	   (setq link
 		 (concat "file:"
 			 (abbreviate-file-name
 			  (buffer-file-name (buffer-base-buffer)))
 			 "::" (match-string 1))
-		 link cpltxt
+                 ;; Target may be shortened when link is inserted.
+                 ;; Avoid [[target][file:~/org/test.org::target]]
+                 ;; links.  Maybe the case of identical target and
+                 ;; description should be handled by `org-insert-link'.
+                 cpltxt nil
+                 desc nil
                  ;; Do not append #CUSTOM_ID link below.
                  custom-id nil))
 	  ((and (featurep 'org-id)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: Bug: org-store-link uses CUSTOM_ID instead of target point [9.4.4 (release_9.4.4 @ /usr/share/emacs/27.2/lisp/org/)]
  2021-11-06 12:51   ` Max Nikulin
@ 2022-08-14  9:58     ` Max Nikulin
  2022-09-13 13:05     ` Ihor Radchenko
  1 sibling, 0 replies; 13+ messages in thread
From: Max Nikulin @ 2022-08-14  9:58 UTC (permalink / raw)
  To: emacs-orgmode

On 06/11/2021 19:51, Max Nikulin wrote:
> On 06/05/2021 19:41, Bastien wrote:
>> Fr Ml writes:
>>
>>> I have a problem with the function org-store-link it doesn't work as
>>> described in the documentation:
>>> https://orgmode.org/manual/Handling-Links.html
>>> "For Org files, if there is a '<<target>>' at point, the link points
>>> to
>>> the target."
>>
>> Fixed in maint, thanks a lot for reporting this and Ihor for
>> confirming the bug.

Another issue likely caused by this change:

Carlos Pita to emacs-orgmode. Adding target and custom id links doesn't 
ask for description. Tue, 2 Aug 2022 14:44:58 -0300.
https://list.orgmode.org/D99A712C-18D1-4A4F-8093-35A0BFB469C4@gmail.com

> Bastien, unfortunately your fix caused duplication of stored links like 
> "file:~/org/file.org::#custom_id" when point is outside of <<target>>. 
> Earlier #CUSTOM_ID link was stored in addition to 
> "file:~/org/file.org::*Heading" search link. My suggestion is to revert 
> your patch and to just reset custom-id variable when <<target>> link is 
> stored. Another effect or your patch, that I consider unintentional, is 
> storing 
> [[file:~/org/file.org::#custom_id][file:~/org/file.org::#custom_id]] 
> instead of [[file:~/org/file.org::#custom_id][Heading]]. I prefer 
> "original" behavior.
> 
> Third patch is intended to avoid links inserted as 
> [[target][file:~/org/file.org::target]] in the case of same file. I 
> suppose, just [[target]] is better. Current variant looks unbalanced and 
> misleading. Of course, you are free to skip last patch.
> 
> I am confused by `org-insert-link' behavior. It inserts links like 
> [[file:~/org/file.org::#custom_id][file:~/org/file.org::#custom_id]] 
> without user prompt for description but 
> [[file:~/org/file.org::#custom_id][Heading]] requires to confirm 
> "Heading" description explicitly. I would expect that "raw" link is 
> subject to ask user for more friendly option.
> 




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Bug: org-store-link uses CUSTOM_ID instead of target point [9.4.4 (release_9.4.4 @ /usr/share/emacs/27.2/lisp/org/)]
  2021-11-06 12:51   ` Max Nikulin
  2022-08-14  9:58     ` Max Nikulin
@ 2022-09-13 13:05     ` Ihor Radchenko
  1 sibling, 0 replies; 13+ messages in thread
From: Ihor Radchenko @ 2022-09-13 13:05 UTC (permalink / raw)
  To: Max Nikulin; +Cc: Bastien, Fr Ml, emacs-orgmode

Max Nikulin <manikulin@gmail.com> writes:

>> Fixed in maint, thanks a lot for reporting this and Ihor for
>> confirming the bug.
>
> Bastien, unfortunately your fix caused duplication of stored links like 
> "file:~/org/file.org::#custom_id" when point is outside of <<target>>. 
> Earlier #CUSTOM_ID link was stored in addition to 
> "file:~/org/file.org::*Heading" search link. My suggestion is to revert 
> your patch and to just reset custom-id variable when <<target>> link is 
> stored. Another effect or your patch, that I consider unintentional, is 
> storing 
> [[file:~/org/file.org::#custom_id][file:~/org/file.org::#custom_id]] 
> instead of [[file:~/org/file.org::#custom_id][Heading]]. I prefer 
> "original" behavior.
>
> Third patch is intended to avoid links inserted as 
> [[target][file:~/org/file.org::target]] in the case of same file. I 
> suppose, just [[target]] is better. Current variant looks unbalanced and 
> misleading. Of course, you are free to skip last patch.

Thanks for reminding about this unresolved patch!
Applied all three patches onto main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=543a23a57d2947cd01c906d134ae9c5c8d0907c4
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=f7b8510283537bda4eba3b54fce5eafc7cec9993
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=c3d6672cfdbff8c9dd4c2ec70886ad3f62153d07

-- 
Ihor Radchenko,
Org mode contributor,
Learn more about Org mode at https://orgmode.org/.
Support Org development at https://liberapay.com/org-mode,
or support my work at https://liberapay.com/yantar92


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-09-13 13:06 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-04 19:17 Bug: org-store-link uses CUSTOM_ID instead of target point [9.4.4 (release_9.4.4 @ /usr/share/emacs/27.2/lisp/org/)] Fr Ml
2021-05-05  2:34 ` Ihor Radchenko
2021-05-06 12:41 ` Bastien
2021-11-06 12:51   ` Max Nikulin
2022-08-14  9:58     ` Max Nikulin
2022-09-13 13:05     ` Ihor Radchenko
2021-05-06 20:08 ` Table alignment problem Fr Ml
2021-05-07 14:50   ` Ihor Radchenko
2021-05-08  6:37     ` Jeremie Juste
2021-05-08  7:45       ` Fr Ml
2021-05-08  7:48     ` Fr Ml
2021-05-08  8:24       ` Ihor Radchenko
     [not found]         ` <5ebfdc6f-6e40-1470-4379-4a5a2b666aa7@t-online.de>
     [not found]           ` <87czu1pivb.fsf@localhost>
     [not found]             ` <6c4c8084-3800-c085-b724-f5653d7c20bd@t-online.de>
     [not found]               ` <87eeeh17fq.fsf@localhost>
     [not found]                 ` <8975eded-5a22-cb9c-b85e-f3b523f16411@t-online.de>
     [not found]                   ` <871ragff66.fsf@localhost>
     [not found]                     ` <84936763-9384-7e6c-5304-b94217298b9a@t-online.de>
     [not found]                       ` <87lf8o9pgn.fsf@localhost>
     [not found]                         ` <eb465a73-691d-01be-135e-f58a9ff36173@t-online.de>
     [not found]                           ` <87im3s9ie8.fsf@localhost>
     [not found]                             ` <2c2f2204-14b1-49b2-658b-2b8b75ab86b9@t-online.de>
2021-05-15  9:26                               ` [PATCH] Pixel-wise table alignment Ihor Radchenko

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).