bug#45837: 28.0.50; incorrect cursor position in visual-line-mode when word-wrap-by-category is t

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#45837: 28.0.50; incorrect cursor position in visual-line-mode when word-wrap-by-category is t

Liu Hui
Steps to reproduce:
1. emacs -Q
2. eval the following code in the *scratch* buffer:

    (with-current-buffer-window "*test*" nil nil
      (dotimes (_ 3)
        (dotimes (i 10)
          (insert "一二三四五六七八九十"))
        (dotimes (i 20)
          (insert "test ")))
      (setq word-wrap-by-category t)
      (visual-line-mode))

3. In the *test* buffer, C-a/C-e don't always move the cursor to the
beginning/end of screen line. There is also an offset between the mouse
click location and the cursor position. If there seems to be no problem,
you can adjust the window width and recheck the cursor movement.


In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.20, cairo version 1.16.0)
of 2021-01-12 built on lcy01-amd64-029
Windowing system distributor 'The X.Org Foundation', version 11.0.12009000
System Description: Ubuntu 20.04.1 LTS

Configured using:
'configure --build=x86_64-linux-gnu --prefix=/usr
'--includedir=${prefix}/include' '--mandir=${prefix}/share/man'
'--infodir=${prefix}/share/info' --sysconfdir=/etc --localstatedir=/var
--disable-silent-rules '--libdir=${prefix}/lib/x86_64-linux-gnu'
'--libexecdir=${prefix}/lib/x86_64-linux-gnu' --disable-maintainer-mode
--disable-dependency-tracking --prefix=/usr --sharedstatedir=/var/lib
--program-suffix=-snapshot --with-modules=yes --with-x=yes
--with-x-toolkit=gtk3 --with-xwidgets=yes 'CFLAGS=-g -O2
-fdebug-prefix-map=/build/emacs-snapshot-4HYkJA/emacs-snapshot-103322=.
-fstack-protector-strong
-Wformat -Werror=format-security' 'CPPFLAGS=-Wdate-time
-D_FORTIFY_SOURCE=2' 'LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro''

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SOUND THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE
XIM XPM XWIDGETS GTK3 ZLIB

Important settings:
value of $LANG: zh_CN.UTF-8
value of $XMODIFIERS: @im=ibus
locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
tooltip-mode: t
global-eldoc-mode: t
eldoc-mode: t
electric-indent-mode: t
mouse-wheel-mode: t
tool-bar-mode: t
menu-bar-mode: t
file-name-shadow-mode: t
global-font-lock-mode: t
font-lock-mode: t
blink-cursor-mode: t
auto-composition-mode: t
auto-encryption-mode: t
auto-compression-mode: t
line-number-mode: t
transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml easymenu mml-sec epa derived epg epg-config gnus-util rmail
rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json map text-property-search time-date
subr-x seq byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs
cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
china-util iso-transl tooltip eldoc electric uniquify ediff-hook
vc-hooks lisp-float-type mwheel term/x-win x-win term/common-win x-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode elisp-mode lisp-mode prog-mode register page
tab-bar menu-bar rfn-eshadow isearch timer select scroll-bar mouse
jit-lock font-lock syntax facemenu font-core term/tty-colors frame
minibuffer cl-generic cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite charscript charprop case-table epa-hook jka-cmpr-hook help
simple abbrev obarray cl-preloaded nadvice button loaddefs faces
cus-face macroexp files window text-properties overlay sha1 md5 base64
format env code-pages mule custom widget hashtable-print-readable
backquote threads xwidget-internal dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 51700 5953)
(symbols 48 6834 1)
(strings 32 18505 2017)
(string-bytes 1 607714)
(vectors 16 12701)
(vector-slots 8 222094 12046)
(floats 8 21 45)
(intervals 56 226 0)
(buffers 984 10))



Reply | Threaded
Open this post in threaded view
|

bug#45837: 28.0.50; incorrect cursor position in visual-line-mode when word-wrap-by-category is t

Eli Zaretskii
> From: Liu Hui <[hidden email]>
> Date: Wed, 13 Jan 2021 10:27:24 +0800
>
> Steps to reproduce:
> 1. emacs -Q
> 2. eval the following code in the *scratch* buffer:
>
>     (with-current-buffer-window "*test*" nil nil
>       (dotimes (_ 3)
>         (dotimes (i 10)
>           (insert "一二三四五六七八九十"))
>         (dotimes (i 20)
>           (insert "test ")))
>       (setq word-wrap-by-category t)
>       (visual-line-mode))
>
> 3. In the *test* buffer, C-a/C-e don't always move the cursor to the
> beginning/end of screen line. There is also an offset between the mouse
> click location and the cursor position. If there seems to be no problem,
> you can adjust the window width and recheck the cursor movement.

Thanks, should be fixed now.



Reply | Threaded
Open this post in threaded view
|

bug#45837: 28.0.50; incorrect cursor position in visual-line-mode when word-wrap-by-category is t

Liu Hui
Eli Zaretskii <[hidden email]> 于2021年1月13日周三 下午10:47写道:

>
> > From: Liu Hui <[hidden email]>
> > Date: Wed, 13 Jan 2021 10:27:24 +0800
> >
> > Steps to reproduce:
> > 1. emacs -Q
> > 2. eval the following code in the *scratch* buffer:
> >
> >     (with-current-buffer-window "*test*" nil nil
> >       (dotimes (_ 3)
> >         (dotimes (i 10)
> >           (insert "一二三四五六七八九十"))
> >         (dotimes (i 20)
> >           (insert "test ")))
> >       (setq word-wrap-by-category t)
> >       (visual-line-mode))
> >
> > 3. In the *test* buffer, C-a/C-e don't always move the cursor to the
> > beginning/end of screen line. There is also an offset between the mouse
> > click location and the cursor position. If there seems to be no problem,
> > you can adjust the window width and recheck the cursor movement.
>
> Thanks, should be fixed now.

Thank you, cursor motion is correct now.

BTW, in the case above, if the line wraps after a non-whitespace
character, C-k does not delete this character. How about the following
change to kill-visual-line?

diff --git a/lisp/simple.el b/lisp/simple.el
index 54c35c04be..86e2c41ac2 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -7337,6 +7337,9 @@ kill-visual-line
       (end-of-visual-line 1)
       (if (= (point) opoint)
          (vertical-motion 1)
+        (when (and word-wrap-by-category
+                   (elt (char-category-set (following-char)) ?|))
+          (forward-char))
        ;; Skip any trailing whitespace at the end of the visual line.
        ;; We used to do this only if `show-trailing-whitespace' is
        ;; nil, but that's wrong; the correct thing would be to check



Reply | Threaded
Open this post in threaded view
|

bug#45837: 28.0.50; incorrect cursor position in visual-line-mode when word-wrap-by-category is t

Eli Zaretskii
> From: Liu Hui <[hidden email]>
> Date: Thu, 14 Jan 2021 12:51:12 +0800
> Cc: [hidden email]
>
> BTW, in the case above, if the line wraps after a non-whitespace
> character, C-k does not delete this character. How about the following
> change to kill-visual-line?

Yes, good catch.  However, this is not entirely right, as the code you
suggest should be _instead_ of skipping the whitespace that follows,
not _in_addition_ to it (imagine that the line is wrapped at a
non-whitespace character followed by whitespace).  And while looking
into adapting your patch to avoid removing more than the user
intended, I found more issues with the existing code.  So I'm
proposing the patch below instead, which should correctly handle the
following use cases:

 . visual line that ends in one or more whitespace characters, and the
   following visual line begins with one or more whitespace characters
 . visual line that ends with a non-whitespace character (under
   word-wrap-by-category) that is followed by one or more whitespace
   characters
 . line that is continued on the next visual line (visual-line-mode is
   off)
 . visual line that is wrapped in the middle of a display string or an
   overlay string with embedded whitespace characters
 . visual line that is wrapped in the middle of intangible text

(The patch also fixes some minor issues with the documentation of this
command.)

WDYT?

diff --git a/lisp/simple.el b/lisp/simple.el
index 54c35c04be..e7421c069f 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -5606,7 +5606,9 @@ zap-to-char
 ;; kill-line and its subroutines.
 
 (defcustom kill-whole-line nil
-  "If non-nil, `kill-line' with no arg at start of line kills the whole line."
+  "If non-nil, `kill-line' with no arg at start of line kills the whole line.
+This variable also affects `kill-visual-line' in the same way as
+it does `kill-line'."
   :type 'boolean
   :group 'killing)
 
@@ -7319,6 +7321,9 @@ kill-visual-line
 If ARG is zero, kill the text before point on the current visual
 line.
 
+If `kill-whole-line' is non-nil, and this command is invoked at
+start of a line that ands in a newline, kill the newline as well.
+
 If you want to append the killed line to the last killed text,
 use \\[append-next-kill] before \\[kill-line].
 
@@ -7331,18 +7336,26 @@ kill-visual-line
   ;; Like in `kill-line', it's better to move point to the other end
   ;; of the kill before killing.
   (let ((opoint (point))
- (kill-whole-line (and kill-whole-line (bolp))))
+        (kill-whole-line (and kill-whole-line (bolp)))
+        (orig-y (cdr (nth 2 (posn-at-point)))))
     (if arg
  (vertical-motion (prefix-numeric-value arg))
       (end-of-visual-line 1)
       (if (= (point) opoint)
   (vertical-motion 1)
- ;; Skip any trailing whitespace at the end of the visual line.
- ;; We used to do this only if `show-trailing-whitespace' is
- ;; nil, but that's wrong; the correct thing would be to check
- ;; whether the trailing whitespace is highlighted.  But, it's
- ;; OK to just do this unconditionally.
- (skip-chars-forward " \t")))
+        ;; The first condition below verifies we are still on the same
+        ;; screen line, i.e. that the line isn't continued, and that
+        ;; end-of-visual-line didn't overshoot due to complications
+        ;; like display or overlay strings, intangible text, etc.:
+        ;; otherwise, we don't want to kill a character that's
+        ;; unrelated to the place where the visual line wrapped.
+        (and (= (cdr (nth 2 (posn-at-point))) orig-y)
+             ;; Make sure we delete the character where the line wraps
+             ;; under visual-line-mode.
+             (or (looking-at-p "[ \t]")
+                 (and word-wrap-by-category
+                      (aref (char-category-set (following-char)) ?\|)))
+             (forward-char))))
     (kill-region opoint (if (and kill-whole-line (= (following-char) ?\n))
     (1+ (point))
   (point)))))



Reply | Threaded
Open this post in threaded view
|

bug#45837: 28.0.50; incorrect cursor position in visual-line-mode when word-wrap-by-category is t

Liu Hui
Eli Zaretskii <[hidden email]> 于2021年1月14日周四 下午9:51写道:

>
> > From: Liu Hui <[hidden email]>
> > Date: Thu, 14 Jan 2021 12:51:12 +0800
> > Cc: [hidden email]
> >
> > BTW, in the case above, if the line wraps after a non-whitespace
> > character, C-k does not delete this character. How about the following
> > change to kill-visual-line?
>
> Yes, good catch.  However, this is not entirely right, as the code you
> suggest should be _instead_ of skipping the whitespace that follows,
> not _in_addition_ to it (imagine that the line is wrapped at a
> non-whitespace character followed by whitespace).  And while looking
> into adapting your patch to avoid removing more than the user
> intended, I found more issues with the existing code.  So I'm
> proposing the patch below instead, which should correctly handle the
> following use cases:
>
>  . visual line that ends in one or more whitespace characters, and the
>    following visual line begins with one or more whitespace characters
>  . visual line that ends with a non-whitespace character (under
>    word-wrap-by-category) that is followed by one or more whitespace
>    characters
>  . line that is continued on the next visual line (visual-line-mode is
>    off)
>  . visual line that is wrapped in the middle of a display string or an
>    overlay string with embedded whitespace characters
>  . visual line that is wrapped in the middle of intangible text
>
> (The patch also fixes some minor issues with the documentation of this
> command.)
>
> WDYT?
>

I have tested the patch and found that the condition `(= (cdr (nth 2
(posn-at-point))) orig-y)` was sometimes too strict. `posn-at-point`
may give slightly different y positions for characters on the same
line when different fonts were used (examples can be found in the
HELLO file). If there are inline graphics (e.g. latex previews), the y
position can also be different.

My suggestion is `(< (abs (- (cdr (nth 2 (posn-at-point))) orig-y))
X)`, where X could be, empirically, `(/ (line-pixel-height) 3)` or
a customizable value.

The patch works well in other cases, thanks!



Reply | Threaded
Open this post in threaded view
|

bug#45837: 28.0.50; incorrect cursor position in visual-line-mode when word-wrap-by-category is t

Eli Zaretskii
> From: Liu Hui <[hidden email]>
> Date: Fri, 15 Jan 2021 15:28:33 +0800
> Cc: [hidden email]
>
> I have tested the patch and found that the condition `(= (cdr (nth 2
> (posn-at-point))) orig-y)` was sometimes too strict. `posn-at-point`
> may give slightly different y positions for characters on the same
> line when different fonts were used (examples can be found in the
> HELLO file). If there are inline graphics (e.g. latex previews), the y
> position can also be different.

Hmm... you are right.  But that sounds like a bug in posn-at-point, I
will look into fixing it soon.

> My suggestion is `(< (abs (- (cdr (nth 2 (posn-at-point))) orig-y))
> X)`, where X could be, empirically, `(/ (line-pixel-height) 3)` or
> a customizable value.

I went with half the line height, thanks.

> The patch works well in other cases, thanks!

Thanks for testing, I've now installed the change on the master
branch.  I'm not yet closing the bug, because I want to see what can
be done about removing the tolerance in comparing values of
posn-at-point.