bug#38851: 27.0.50; Recent my patch breaks isearch with macOS native input method

bug#38851: 27.0.50; Recent my patch breaks isearch with macOS native input method

[tsuucat via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

I sent a patch for Bug#23412 and the patch was already merged.
(http://git.savannah.gnu.org/cgit/emacs.git/commit/?h=emacs-27&id=ba042176d8931cdf9441b3b4919ec74b75019494)

Unfortunately, the patch breaks isearch with macOS native input method.
I tried to modify ns-echo-working-text function and ns-delete-working-text
function, but I cannot do it.
This is because input-pending-p now returns t after pressing RET to confirm 
the conversion. (isearch-update function uses input-pending-p)

The following patch treats ns-unput-working-text event by deleteWorkingText 
specially in read_char(). This solve the problem.


[-- Attachment #2: 0001-Change-redisplay-solution.patch --]
[-- Type: application/octet-stream, Size: 1756 bytes --]

From 9f337cfc6c5daea484b4e1d42d20e017e798cdee Mon Sep 17 00:00:00 2001
From: Masahiro Nakamura <tsuucat@icloud.com>
Date: Thu, 2 Jan 2020 01:21:40 +0900
Subject: [PATCH] Change redisplay solution

---
 src/keyboard.c | 6 ++++++
 src/nsterm.m   | 8 ++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index 7d3b024..30fd526 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2896,6 +2896,12 @@ read_char (int commandflag, Lisp_Object map,
 	   example banishing the mouse under mouse-avoidance-mode.  */
 	timer_resume_idle ();
 
+#ifdef HAVE_NS
+      if (CONSP (c)
+          && (EQ (XCAR (c), intern ("ns-unput-working-text"))))
+        input_was_pending = input_pending;
+#endif
+
       if (current_buffer != prev_buffer)
 	{
 	  /* The command may have changed the keymaps.  Pretend there
diff --git a/src/nsterm.m b/src/nsterm.m
index c5cc182..52a9830 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -6430,6 +6430,10 @@ - (void)insertText: (id)aString
   if (!emacs_event)
     return;
 
+  /* First, clear any working text.  */
+  if (workingText != nil)
+    [self deleteWorkingText];
+
   /* It might be preferable to use getCharacters:range: below,
      cf. https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CocoaPerformance/Articles/StringDrawing.html#//apple_ref/doc/uid/TP40001445-112378.
      However, we probably can't use SAFE_NALLOCA here because it might
@@ -6458,10 +6462,6 @@ - (void)insertText: (id)aString
       emacs_event->code = code;
       EV_TRAILER ((id)nil);
     }
-
-  /* Last, clear any working text.  */
-  if (workingText != nil)
-    [self deleteWorkingText];
 }
 
 
-- 
2.21.0


[-- Attachment #3: Type: text/plain, Size: 11 bytes --]


--
tsuucat

bug#38851: 27.0.50; Recent my patch breaks isearch with macOS native input method

[Alan Third]

On Thu, Jan 02, 2020 at 03:25:13AM +0900, tsuucat via Bug reports for GNU Emacs, the Swiss army knife of text editors wrote:
> I sent a patch for Bug#23412 and the patch was already merged.
> (http://git.savannah.gnu.org/cgit/emacs.git/commit/?h=emacs-27&id=ba042176d8931cdf9441b3b4919ec74b75019494)
> 
> Unfortunately, the patch breaks isearch with macOS native input method.
> I tried to modify ns-echo-working-text function and ns-delete-working-text
> function, but I cannot do it.
> This is because input-pending-p now returns t after pressing RET to confirm 
> the conversion. (isearch-update function uses input-pending-p)
> 
> The following patch treats ns-unput-working-text event by deleteWorkingText 
> specially in read_char(). This solve the problem.

Thanks. I have no better ideas, so this will have to do. Does anyone
else have any opinions?
-- 
Alan Third

bug#38851: 27.0.50; Recent my patch breaks isearch with macOS native input method

[tsuucat via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


> Thanks. I have no better ideas, so this will have to do. Does anyone
> else have any opinions?

I have some more explanation. After this commit (https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=9e77c1b7bcfd0807be7fe67daf73c2320e864309 <https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=9e77c1b7bcfd0807be7fe67daf73c2320e864309>)
which introduce input_was_pending, read_char became to redisplay always 
after processing special event.

```
input_was_pending = input_pending;
```
is a way to prevent redisplay. The same code is placed at end of read_char.


Emacs at current emacs-27 branch cannot use isearch with macOS native 
input method (I checked with macOS Japanese input method and get 
```
ns-delete-working-text: Wrong type argument: arrayp, nil
```
).  This is worse situation for macOS native input method users.
Even if modifying keyboard.c is not acceptable, I think reverting the position of 
```
  if (workingText != nil)
    [self deleteWorkingText];
```
in insertText should be done.

If the patch I sent before is acceptable, I have two patch for improvement.

0001-proper-selection-window-position-for-isearch.patch
This is to show candidate window for IM at proper position when isearch.


0001-Remove-unfavorable-deleteWorkingText.patch
Currently clicking buffer or focusing out frame delete working text. To prevent
deleting working text by such a input.


--
 tsuucat

[-- Attachment #2.1: Type: text/html, Size: 2263 bytes --]

[-- Attachment #2.2: 0001-proper-selection-window-position-for-isearch.patch --]
[-- Type: application/octet-stream, Size: 1166 bytes --]

From db2b26d0891e60d67cad68982af44d759b407a58 Mon Sep 17 00:00:00 2001
From: Masahiro Nakamura <tsuucat@icloud.com>
Date: Sat, 4 Jan 2020 20:19:42 +0900
Subject: [PATCH] proper selection window position for isearch

---
 src/nsterm.m | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/nsterm.m b/src/nsterm.m
index 52a9830..7674fb0 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -6557,13 +6557,18 @@ - (NSRect)firstRectForCharacterRange: (NSRange)theRange
 {
   NSRect rect;
   NSPoint pt;
-  struct window *win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));
+  struct window *win;
 
   NSTRACE ("[EmacsView firstRectForCharacterRange:]");
 
   if (NS_KEYLOG)
     NSLog (@"firstRectForCharRange request");
 
+  if (WINDOWP (echo_area_window) && ! NILP (call0 (intern ("ns-in-echo-area"))))
+    win = XWINDOW (echo_area_window);
+  else
+    win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));
+
   rect.size.width = theRange.length * FRAME_COLUMN_WIDTH (emacsframe);
   rect.size.height = FRAME_LINE_HEIGHT (emacsframe);
   pt.x = WINDOW_TEXT_TO_FRAME_PIXEL_X (win, win->phys_cursor.x);
-- 
2.21.0


[-- Attachment #2.3: Type: text/html, Size: 494 bytes --]

[-- Attachment #2.4: 0001-Remove-unfavorable-deleteWorkingText.patch --]
[-- Type: application/octet-stream, Size: 834 bytes --]

From 3a5c22dfc70197cce8cc42680d500a56ba8146b3 Mon Sep 17 00:00:00 2001
From: Masahiro Nakamura <tsuucat@icloud.com>
Date: Tue, 11 Feb 2020 01:51:14 +0900
Subject: [PATCH] Remove unfavorable deleteWorkingText

---
 src/nsterm.m | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/nsterm.m b/src/nsterm.m
index 7674fb0..c7450b4 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -6675,8 +6675,6 @@ - (void)mouseDown: (NSEvent *)theEvent
 
   NSTRACE ("[EmacsView mouseDown:]");
 
-  [self deleteWorkingText];
-
   if (!emacs_event)
     return;
 
@@ -7301,7 +7299,6 @@ - (void)windowDidResignKey: (NSNotification *)notification
 
   if (emacs_event && is_focus_frame)
     {
-      [self deleteWorkingText];
       emacs_event->kind = FOCUS_OUT_EVENT;
       EV_TRAILER ((id)nil);
     }
-- 
2.21.0


[-- Attachment #2.5: Type: text/html, Size: 314 bytes --]

bug#38851: 27.0.50; Recent my patch breaks isearch with macOS native input method

[Alan Third]

On Tue, Feb 11, 2020 at 02:08:14AM +0900, tsuucat wrote:
> 
> > Thanks. I have no better ideas, so this will have to do. Does anyone
> > else have any opinions?

Apologies, I forgot about this patch. I can see two problems here:

You probably need to sign the copyright assignment form before any
further patches can be applied.

It’s getting pretty far on in the Emacs 27 release cycle and I may
have left this too late.

IIRC your patch fixed a visual flicker rather than any actual
usability problem, is that right? Would we be better reverting it in
Emacs 27 and continuing with it and these further patches in the
master branch?

Eli, any opinion on this? The commit in question is

    ba042176d8931cdf9441b3b4919ec74b75019494
-- 
Alan Third

bug#38851: 27.0.50; Recent my patch breaks isearch with macOS native input method

[tsuucat via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> You probably need to sign the copyright assignment form before any
> further patches can be applied.

I have already assigned copyright for GNU Emacs.

> IIRC your patch fixed a visual flicker rather than any actual
> usability problem, is that right?


Yes.

--
tsuucat

bug#38851: 27.0.50; Recent my patch breaks isearch with macOS native input method

[Eli Zaretskii]

> Date: Tue, 11 Feb 2020 07:21:49 +0900
> From: tsuucat via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > You probably need to sign the copyright assignment form before any
> > further patches can be applied.
> 
> I have already assigned copyright for GNU Emacs.

Indeed, the assignment is already on file.

Thanks.

bug#38851: 27.0.50; Recent my patch breaks isearch with macOS native input method

[Eli Zaretskii]

> Date: Mon, 10 Feb 2020 21:44:51 +0000
> From: Alan Third <alan@idiocy.org>
> Cc: 38851@debbugs.gnu.org
> 
> Eli, any opinion on this? The commit in question is
> 
>     ba042176d8931cdf9441b3b4919ec74b75019494

I don't think I understand the change well enough, as it deals with NS
specific code.  But if it didn't fix the actual problem, perhaps it is
indeed better to work on it on master.

bug#38851: 27.0.50; Recent my patch breaks isearch with macOS native input method

[Alan Third]

On Tue, Feb 11, 2020 at 07:21:49AM +0900, tsuucat wrote:
> 
> > You probably need to sign the copyright assignment form before any
> > further patches can be applied.
> 
> I have already assigned copyright for GNU Emacs.

Thanks.

> > IIRC your patch fixed a visual flicker rather than any actual
> > usability problem, is that right?
> 
> 
> Yes.

OK, I want to revert the patch on the Emacs 27 branch. This means the
flicker will be on Emacs 27, but at least it will be usable.

Please squash your three new patches together and have a look at the
instructions in CONTRIBUTE regarding commit messages.

Once you’ve done that I’ll push them to the master branch. Your
original patch will still be on master, so there’s no need to include
it.

Thanks for working on this and sorry again for the delay.
-- 
Alan Third

bug#38851: 27.0.50; Recent my patch breaks isearch with macOS native input method

[tsuucat via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


> Please squash your three new patches together and have a look at the
> instructions in CONTRIBUTE regarding commit messages.

OK. I squashed patches and attach the patch. Please tell me if there’s 
something wrong.

[-- Attachment #2: 0001-Fix-working-text-related-issues-on-NS-Bug-38851.patch --]
[-- Type: application/octet-stream, Size: 3316 bytes --]

From 01250b154fc19c7dc22d70907f3089ad4b43244d Mon Sep 17 00:00:00 2001
From: Masahiro Nakamura <tsuucat@icloud.com>
Date: Fri, 14 Feb 2020 03:21:15 +0900
Subject: [PATCH] Fix working text related issues on NS (Bug#38851)

* src/keyboard.c (read_char): Prevent redsiplay right after
ns-unput-working-text event.
* src/nsterm.m ([EmacsView insertText:]): Partially revert commit
ba04217.
([EmacsView firstRectForCharacterRange:]): Fix candidate window
position when cursor is on echoarea.
([EmacsView mouseDown:])
([EmacsView windowDidResignKey:]): Don't delete working text.
---
 src/keyboard.c |  6 ++++++
 src/nsterm.m   | 18 ++++++++++--------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index bf1f5da..9dd7e00 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2901,6 +2901,12 @@ read_char (int commandflag, Lisp_Object map,
 	   example banishing the mouse under mouse-avoidance-mode.  */
 	timer_resume_idle ();
 
+#ifdef HAVE_NS
+      if (CONSP (c)
+          && (EQ (XCAR (c), intern ("ns-unput-working-text"))))
+        input_was_pending = input_pending;
+#endif
+
       if (current_buffer != prev_buffer)
 	{
 	  /* The command may have changed the keymaps.  Pretend there
diff --git a/src/nsterm.m b/src/nsterm.m
index 9d427b9..fddd4b0 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -6473,6 +6473,10 @@ - (void)insertText: (id)aString
   if (!emacs_event)
     return;
 
+  /* First, clear any working text.  */
+  if (workingText != nil)
+    [self deleteWorkingText];
+
   /* It might be preferable to use getCharacters:range: below,
      cf. https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CocoaPerformance/Articles/StringDrawing.html#//apple_ref/doc/uid/TP40001445-112378.
      However, we probably can't use SAFE_NALLOCA here because it might
@@ -6501,10 +6505,6 @@ - (void)insertText: (id)aString
       emacs_event->code = code;
       EV_TRAILER ((id)nil);
     }
-
-  /* Last, clear any working text.  */
-  if (workingText != nil)
-    [self deleteWorkingText];
 }
 
 
@@ -6600,13 +6600,18 @@ - (NSRect)firstRectForCharacterRange: (NSRange)theRange
 {
   NSRect rect;
   NSPoint pt;
-  struct window *win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));
+  struct window *win;
 
   NSTRACE ("[EmacsView firstRectForCharacterRange:]");
 
   if (NS_KEYLOG)
     NSLog (@"firstRectForCharRange request");
 
+  if (WINDOWP (echo_area_window) && ! NILP (call0 (intern ("ns-in-echo-area"))))
+    win = XWINDOW (echo_area_window);
+  else
+    win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));
+
   rect.size.width = theRange.length * FRAME_COLUMN_WIDTH (emacsframe);
   rect.size.height = FRAME_LINE_HEIGHT (emacsframe);
   pt.x = WINDOW_TEXT_TO_FRAME_PIXEL_X (win, win->phys_cursor.x);
@@ -6713,8 +6718,6 @@ - (void)mouseDown: (NSEvent *)theEvent
 
   NSTRACE ("[EmacsView mouseDown:]");
 
-  [self deleteWorkingText];
-
   if (!emacs_event)
     return;
 
@@ -7341,7 +7344,6 @@ - (void)windowDidResignKey: (NSNotification *)notification
 
   if (emacs_event && is_focus_frame)
     {
-      [self deleteWorkingText];
       emacs_event->kind = FOCUS_OUT_EVENT;
       EV_TRAILER ((id)nil);
     }
-- 
2.21.0


[-- Attachment #3: Type: text/plain, Size: 88 bytes --]


> Thanks for working on this and sorry again for the delay.

Thank you too!

--
tsuucat

bug#38851: 27.0.50; Recent my patch breaks isearch with macOS native input method

[Alan Third]

On Fri, Feb 14, 2020 at 04:17:29AM +0900, tsuucat wrote:
> 
> > Please squash your three new patches together and have a look at the
> > instructions in CONTRIBUTE regarding commit messages.
> 
> OK. I squashed patches and attach the patch. Please tell me if there’s 
> something wrong.

Everything looks good to me, thank you. I’ve pushed to master.
-- 
Alan Third