[PATCH] org: Fix tag width calculation for multi-column chars

[PATCH] org: Fix tag width calculation for multi-column chars

[Yasushi SHOJI]

Some characters have multiple column width.  Calculating string width
with points gives a wrong value than actual display width.  Use
`string-width' instead.

Here is an ECM for this problem.  `M-x org-update-statistics-cookies` or
`C-c #` on bar moves the tag on the headline.

* foo [0/0]								:abc:
** child
* bar [0/0]							     :日本語:
** child
12345678901234567890123456789012345678901234567890123456789012345678901234567890
         1         2         3         4         5         6         7         8

Simple test case is also included.
---

Hello,

While using Japanese as a tag, it seems to move the position of it
when `org-align-tags-here' is called.

If Japanese characters are allowed for tags, please merge it.

Let me know if I miss something.

Thanks,
            yashi


 lisp/org.el              |  2 +-
 testing/lisp/test-org.el | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/lisp/org.el b/lisp/org.el
index e4edcf4..d34163f 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -14922,7 +14922,7 @@ If ONOFF is `on' or `off', don't toggle but set to this state."
     (if	(and (looking-at (org-re ".*?\\([ \t]+\\)\\(:[[:alnum:]_@#%:]+:\\)[ \t]*$"))
 	     (< pos (match-beginning 2)))
 	(progn
-	  (setq tags-l (- (match-end 2) (match-beginning 2)))
+	  (setq tags-l (string-width (match-string 2)))
 	  (goto-char (match-beginning 1))
 	  (insert " ")
 	  (delete-region (point) (1+ (match-beginning 2)))
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 73245b4..7de0fa0 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -4406,6 +4406,29 @@ Paragraph<point>"
 		   (funcall list-visible-lines 'canonical nil)))))
 
 
+\f
+;;; Tags
+
+(ert-deftest test-org/tag-align ()
+  "Test `org-align-tags-here' with different display width."
+  (should
+   ;;      12345678901234567890
+   (equal "* Test         :abc:"
+	  (org-test-with-temp-text "* Test :abc:"
+	    (let ((org-tags-column -20)
+		  (indent-tabs-mode nil))
+	     (org-fix-tags-on-the-fly))
+	    (buffer-string))))
+  (should
+   ;;      12345678901234567890
+   (equal "* Test      :日本語:"
+	  (org-test-with-temp-text "* Test :日本語:"
+	    (let ((org-tags-column -20)
+		  (indent-tabs-mode nil))
+	     (org-fix-tags-on-the-fly))
+	    (buffer-string)))))
+
+
 (provide 'test-org)
 
 ;;; test-org.el ends here
-- 
2.8.1

Re: [PATCH] org: Fix tag width calculation for multi-column chars

[Nicolas Goaziou]

Hello,

Yasushi SHOJI <yashi@atmark-techno.com> writes:

> Some characters have multiple column width.  Calculating string width
> with points gives a wrong value than actual display width.  Use
> `string-width' instead.
>
> Here is an ECM for this problem.  `M-x org-update-statistics-cookies` or
> `C-c #` on bar moves the tag on the headline.
>
> * foo [0/0]								:abc:
> ** child
> * bar [0/0]							     :日本語:
> ** child
> 12345678901234567890123456789012345678901234567890123456789012345678901234567890
>          1         2         3         4         5         6         7         8
>
> Simple test case is also included.

Thank you.

> Let me know if I miss something.

Could you send it again with an appropriate commit message and using git
format-patch?

Also, there is already a "Tags" section in "test-org.el". Could you add
your tests there instead of creating a new section?

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] org: Fix tag width calculation for multi-column chars

[Samuel Wales]

i wonder if this also fixes tags for variable pitch (proportional)
fonts.  i have been having great success with variable pitch fonts and
it would be great if this fixes tags for them too.

-- 
The Kafka Pandemic: http://thekafkapandemic.blogspot.com

The disease DOES progress.  MANY people have died from it.  And
ANYBODY can get it.

Denmark: free Karina Hansen NOW.

Re: org: Fix tag width calculation for multi-column chars

[Georgiy Tugai]

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

On 31 May, Samuel Wales wrote:
> i wonder if this also fixes tags for variable pitch (proportional)
> fonts.  i have been having great success with variable pitch fonts and
> it would be great if this fixes tags for them too.
> 
> -- 
> The Kafka Pandemic: http://thekafkapandemic.blogspot.com
> 
> The disease DOES progress.  MANY people have died from it.  And
> ANYBODY can get it.
> 
> Denmark: free Karina Hansen NOW.
> 

I'm not sure if this is the right place to ask, but would it be possible
to implement, for lack of better terms, "virtual" tag positioning? In
other words, the tags in the actual file are placed as usual
(org-tags-column), but if a new variable (say, org-tags-column-display)
is set, an overlay is placed onto each tag which results in the tags
being shown at that location rather than where they actually are in the
line?

I understand that this violates "what you see is what is in the file";
however, this would allow (I believe) easier handling of windows where
org-tags-column is wider than the window, as well as variable font sizes
(f.e. some themes use large font sizes for headings, which interferes
with tag positioning).

If this can be done and doesn't violate any of Org's design principles,
I would appreciate some pointers on where to hook into Org to implement
such a mechanism.

Regards,
Georgiy

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: org: Fix tag width calculation for multi-column chars

[Yasushi SHOJI]

On Wed, 01 Jun 2016 14:57:04 +0900,
Georgiy Tugai wrote:
> 
> I'm not sure if this is the right place to ask, but would it be possible
> to implement, for lack of better terms, "virtual" tag positioning? In
> other words, the tags in the actual file are placed as usual
> (org-tags-column), but if a new variable (say, org-tags-column-display)
> is set, an overlay is placed onto each tag which results in the tags
> being shown at that location rather than where they actually are in the
> line?

Or, right align to the edge of your window would be nice. 

> I understand that this violates "what you see is what is in the file";
> however, this would allow (I believe) easier handling of windows where
> org-tags-column is wider than the window, as well as variable font sizes
> (f.e. some themes use large font sizes for headings, which interferes
> with tag positioning).
> 
> If this can be done and doesn't violate any of Org's design principles,
> I would appreciate some pointers on where to hook into Org to implement
> such a mechanism.

I'm not an expart but links in org document uses text property for
display?
-- 
            yashi

Re: [PATCH] org: Fix tag width calculation for multi-column chars

[Yasushi SHOJI]

Hi Samuel,

On Wed, 01 Jun 2016 06:06:02 +0900,
Samuel Wales wrote:
> 
> i wonder if this also fixes tags for variable pitch (proportional)
> fonts.  i have been having great success with variable pitch fonts and
> it would be great if this fixes tags for them too.

This fix is nothing to do with propotional fonts.  What kind of
problem do you have right now?  Any screen shot or an ECM?

If you are refering to the Ambiguous width defined in UAX#11, it might.
-- 
           yashi

Re: [PATCH] org: Fix tag width calculation for multi-column chars

[Rasmus]

Samuel Wales <samologist@gmail.com> writes:

> i wonder if this also fixes tags for variable pitch (proportional)
> fonts.  i have been having great success with variable pitch fonts and
> it would be great if this fixes tags for them too.

No.

    https://www.gnu.org/software/emacs/manual/html_node/elisp/Size-of-Displayed-Text.html


You'd need pixel alignment.

Here’s a version that aligns the left part pf tags.

    (defun org-align-tags-here (&optional arg)
      ;; Assumes that this is a headline
      "Align tags on the current headline to TO-COL."
      (save-excursion
        (when (org-at-heading-p)
          (beginning-of-line)
          (re-search-forward org-complex-heading-regexp (line-end-position) t)
          (when (match-end 5) ; There’s a tag.
            (goto-char (match-end 4))
            (delete-region (point) (match-beginning 5))
            (insert (propertize
                     (make-string (- (abs org-tags-column) (current-column)
                                     (- (match-end 5) (match-beginning 5)))
                                  ?\s)
                     ;; one would need somehow intelligently figure out
                     ;; the maximum pixel number.
                     'display '(space :align-to (600))))))))


Questions if we were to pixel align:

- Can we drop right alignment of tags? (i.e. org-tags-column < 0).
- We might need to know the pixel width of :tags: anyway to set the width
  of the white space to "right-margin" - "width-of-tags".  Is this robust
  to resizing?  Or is there a "\hfill"-like property for Emacs space?
- How to determine max width on "auto-filled" file?  Go through each line
  or just make it a custom value?

Perhaps a nice "conservative" way is to introduce a org-tags-column-pixel,
but if we can find a good "automatic" way that’s nicer IMO.

Rasmus

-- 
The second rule of Fight Club is: You do not talk about Fight Club

Re: [PATCH] org: Fix tag width calculation for multi-column chars

[Yasushi SHOJI]

Hi Nicolas,

On Wed, 01 Jun 2016 05:02:58 +0900,
Nicolas Goaziou wrote:
> Yasushi SHOJI <yashi@atmark-techno.com> writes:
> > Let me know if I miss something.
> 
> Could you send it again with an appropriate commit message and using git
> format-patch?

Oops.  Sorry about the format=flowed.  Not sure how I got that, but
might be gmail thing.  I'll use NNTP or git pull request next time.

> Also, there is already a "Tags" section in "test-org.el". Could you add
> your tests there instead of creating a new section?

Ah, the patch was intended for the branch maint.

Would you remove the extra section marker (";;; Tags") thing when
merge to master?  It merges cleanly from maint to master but leaves
the marker.

please pull from:

  https://github.com/yashi/org-mode tag-width-fix

ps. If you want me to change it for master, let me know.  I'll do it.
--
            yashi

Re: [PATCH] org: Fix tag width calculation for multi-column chars

[Nicolas Goaziou]

Hello,

Yasushi SHOJI <yashi@atmark-techno.com> writes:

> Ah, the patch was intended for the branch maint.

You're right, Tags section doesn't exist in in maint, so all is good.

> Would you remove the extra section marker (";;; Tags") thing when
> merge to master?  It merges cleanly from maint to master but leaves
> the marker.

I'll take care of the merging back to master.

> please pull from:
>
>   https://github.com/yashi/org-mode tag-width-fix

I'd rather have the patch with the commit message as an attachment, if
you don't mind.

Thank you !


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] org: Fix tag width calculation for multi-column chars

[Yasushi SHOJI]

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

Hi,

On Thu, 02 Jun 2016 02:34:31 +0900,
Nicolas Goaziou wrote:
> 
> > Would you remove the extra section marker (";;; Tags") thing when
> > merge to master?  It merges cleanly from maint to master but leaves
> > the marker.
> 
> I'll take care of the merging back to master.

Thank you.

> > please pull from:
> >
> >   https://github.com/yashi/org-mode tag-width-fix
> 
> I'd rather have the patch with the commit message as an attachment, if
> you don't mind.

Sure.  Attached.

Thanks,
-- 
         yashi


[-- Attachment #2: 0001-org-Fix-tag-width-calculation-for-multi-column-chars.patch --]
[-- Type: text/plain, Size: 1459 bytes --]

From b57dee1c44d8dd6979c5a6953ad12fdb487d5092 Mon Sep 17 00:00:00 2001
From: Yasushi SHOJI <yashi@atmark-techno.com>
Date: Tue, 31 May 2016 16:25:42 +0900
Subject: [PATCH 1/2] org: Fix tag width calculation for multi-column chars
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Some characters have multiple column width.  Calculating string width
with points gives a wrong value than actual display width.  Use
`string-width' instead.

Here is an ECM for this problem.  `M-x org-update-statistics-cookies` or
`C-c #` on bar moves the tag on the headline.

* foo [0/0]								:abc:
** child
* bar [0/0]							     :日本語:
** child
12345678901234567890123456789012345678901234567890123456789012345678901234567890
         1         2         3         4         5         6         7         8
---
 lisp/org.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/org.el b/lisp/org.el
index a9412ea..15f851d 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -14923,7 +14923,7 @@ If ONOFF is `on' or `off', don't toggle but set to this state."
     (if	(and (looking-at (org-re ".*?\\([ \t]+\\)\\(:[[:alnum:]_@#%:]+:\\)[ \t]*$"))
 	     (< pos (match-beginning 2)))
 	(progn
-	  (setq tags-l (- (match-end 2) (match-beginning 2)))
+	  (setq tags-l (string-width (match-string 2)))
 	  (goto-char (match-beginning 1))
 	  (insert " ")
 	  (delete-region (point) (1+ (match-beginning 2)))
-- 
2.8.1


[-- Attachment #3: 0002-org-Add-test-for-tag-width-calculation.patch --]
[-- Type: text/plain, Size: 1319 bytes --]

From 4cbd72931e7982878334c92abb08272f11ae0f32 Mon Sep 17 00:00:00 2001
From: Yasushi SHOJI <yashi@atmark-techno.com>
Date: Tue, 31 May 2016 16:25:42 +0900
Subject: [PATCH 2/2] org: Add test for tag width calculation

This is a simple unit test case for the previous fix.
---
 testing/lisp/test-org.el | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index db7e525..4873fc2 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -4132,6 +4132,28 @@ Paragraph<point>"
 	(org-occur "A" nil (lambda () (equal (org-get-heading) "H2")))))))
 
 \f
+;;; Tags
+
+(ert-deftest test-org/tag-align ()
+  "Test `org-align-tags-here' with different display width."
+  (should
+   ;;      12345678901234567890
+   (equal "* Test         :abc:"
+	  (org-test-with-temp-text "* Test :abc:"
+	    (let ((org-tags-column -20)
+		  (indent-tabs-mode nil))
+	     (org-fix-tags-on-the-fly))
+	    (buffer-string))))
+  (should
+   ;;      12345678901234567890
+   (equal "* Test      :日本語:"
+	  (org-test-with-temp-text "* Test :日本語:"
+	    (let ((org-tags-column -20)
+		  (indent-tabs-mode nil))
+	     (org-fix-tags-on-the-fly))
+	    (buffer-string)))))
+
+\f
 ;;; Timestamps API
 
 (ert-deftest test-org/time-stamp ()
-- 
2.8.1

Re: [PATCH] org: Fix tag width calculation for multi-column chars

[Nicolas Goaziou]

Hello,

Yasushi SHOJI <yashi@atmark-techno.com> writes:

> Sure.  Attached.

Applied. Thank you.

Regards,

-- 
Nicolas Goaziou