tcode-use-input-method のバグ修正と isearch 中の部首/交ぜ書き変換の実装#32
Open
tooro88 wants to merge 11 commits into
Open
Conversation
Author
|
#30 (advice 方式) で commit を分割したので、こちらも更新になります。こちらも少し commit を分割し、 元の は、次の4つになりました。
テストは、#30 の方の commit にまとめました(元々理由があって分けていたのではなく、PR作成タイミングの都合で分かれていただけなので)。 |
tooro88
force-pushed
the
isearch-im
branch
2 times, most recently
from
c6f14cd to
c429365
Compare
Author
|
バグを見つけたので、commit を修正しました。
|
tooro88
force-pushed
the
isearch-im
branch
2 times, most recently
from
7ecf50b to
e3b4ca6
Compare
|
お待たせしています. こちらもレビュー開始したいと思います. おそらく, Handle errors in input method to prevent Isearch breakage からが今回分のcommitだと思います. いまの masterに rebaseにしてからpushしていただけると, 以前のPR分が消えて今回分のだけになるかと思います. |
When `tcode-use-input-method` is non-`nil`, errors in `tcode-input-method` can leave Isearch in an unrecoverable state. Catch errors with condition-case and discard them after displaying the error message.
When `tcode-use-input-method` is non-`nil`, a keyboard macro whose final key invokes bushu conversion does not terminate automatically; it terminates only after additional user input is supplied. This occurs because `tcode-input-method` returns an empty event list after performing bushu conversion. The top-level command loop in Emacs contains an inner loop that repeatedly calls the input method as long as it returns an empty event list. This loop continues even after the keyboard macro has finished, causing Emacs to wait for user input. This issue can be triggered by any T-Code subcommand, not just bushu conversion. For the same reason, `post-command-hook` is not invoked immediately after executing a subcommand. Fix `tcode-input-method` so that it never returns `nil` as an event list. Instead, return a dummy event `tcode-ignore`, bound to the no-op command `ignore`. In addition, `tcode-ignore` now acts as an undo boundary that isolates a bushu/mazegaki conversion from subsequent insertions. Without this commit, such a boundary would otherwise need to be implemented separately to prevent unexpected undo amalgamation.
Add an `'im` option to `tcode-use-isearch`. When this option is selected, `tcode-use-input-method` is also set to `t` automatically, thus enabling T-Code within Isearch.
When `tcode-use-input-method` is non-`nil`, the result of postfix-bushu/mazegaki conversion in Isearch is discarded by the caller of the input method. Add a post-processing phase that updates the Isearch state stack with the conversion result.
When `tcode-use-input-method` is non-`nil`, Isearch prepares a minibuffer for invoking the input method. However, that minibuffer is not suitable for prefix-bushu/mazegaki conversion, because it exits and discards its contents after each character input. Instead of using that minibuffer, prepare a dedicated one for the conversion, which remains active until the entire conversion is completed.
When `tcode-use-input-method` is non-`nil`, Isearch invokes `tcode-input-method` in the minibuffer. This prevents the function from referring to buffer-local variables in the editing buffer. Since various T-Code modes use buffer-local variables to maintain their state, the input method cannot determine which mode was previously active. Moreover, any mode changes made in the minibuffer are immediately lost because the minibuffer is deactivated each time `tcode-input-method` returns. Copy the buffer-local variables that hold mode state to the minibuffer when it is activated, and write back the modified values when it is deactivated. This allows the input method to behave correctly according to the state of the editing buffer, and ensures that mode changes persist across successive activations of the minibuffer.
An issue regarding `last-command` modification will be fixed by the next commit. Remove `:expected-result :failed` annotations from the related tests.
Typing "a BACKSPACE b" and invoking undo once should delete only "a". However, the undo records for BACKSPACE and "b" are amalgamated, so both are undone at once. This bug occurs with the `japanese-2byte-alnum` input method (`tc-ja-alnum.el`), and with the T-Code input method when `tcode-use-input-method` is enabled. Both modify `last-command` to `self-insert-command` for unknown reasons. As a result, when "b" is typed, BACKSPACE is treated as an insertion and amalgamated with the insertion of "b". Remove two `setq` forms in `japanese-2byte-alnum`: one that sets `last-command` and another that sets `last-command-event`, which also seems unnecessary. For T-Coded, remove only the `setq` for `last-command`. The variable `last-command-event` is still used by subcommands called within, such as `tcode-mazegaki-self-insert-or-convert`, which eventually calls `self-insert-command` and requires `last-command-event` to be set properly. The only possible reason I could find for setting `last-command` is that `tc-comelete.el` refers to it. However, this seems to be a bug: the code uses `last-command` to determine the last command executed by the user, where it should instead use `this-command`. In any case, `tc-comelete.el` is currently broken and would require a complete rework to become usable again, so the impact of this change can be safely ignored for now.
The following bugs, which occur when `tcode-use-input-method` is non-`nil`, stem from the same cause and are fixed by this commit: - After `indent-rigidly`, typing an ordinary character normally exits the special mode and inserts that character, so editing can resume immediately. The same should happen when inputting a Japanese character, but instead an ASCII character appears as if the input method were disabled. - Inputting a Japanese character also produces an ASCII character when a prefix argument such as `C-u` or `M-3` is used. Both `indent-rigidly` and commands that set prefix arguents (such as `universal-argument`) call `set-transient-map`, which in turn activates `overriding-terminal-local-map`. When `overriding-terminal-local-map` is active, `tcode-input-method` stops processing input events entirely and treats them as ASCII characters. Fix the handling of `overriding-terminal-local-map` in `tcode-input-method` to match the behavior of `quail-input-method`: it now stops processing the input event only if the event is bound in `overriding-terminal-local-map`. See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=68338 for the discussion of the same issue in the Quail input method.
After mazegaki conversion has been used once, inserting a space character (SPC) becomes affected by the prefix argument of the previous command, rather than the current one. When mazegaki conversion is used for the first time, `tc-mazegaki.el` adds a binding for SPC to `tcode-mode-map`. Thereafter, typing SPC is handled by `tcode-input-method`, which executes the command `tcode-mazegaki-space-or-convert` with the value of `current-prefix-arg` as its prefix argument. This issue occurs when `tcode-use-input-method` is non-`nil`. Changing this variable affects when `tcode-input-method` is invoked. When the value is `nil`, the function is called from `tcode-self-insert-command`, at which point `current-prefix-arg` correctly represents the prefix argument for `tcode-self-insert-command`, so it is appropriate to pass it along to the subcommand. However, when `tcode-use-input-method` is non-`nil`, `tcode-input-method` is invoked directly from the event loop, at a time when no command is currently executing. In that case, `current-prefix-arg` still holds the prefix argument from the previous command, so using it for the subcommand is incorrect. Use `prefix-argument` instead when `tcode-use-input-method` is non-`nil`.
Author
|
前 PR のマージありがとうございました！！＆引き続きよろしくお願いします！！ すいません、rebase 確かにやっておくべきでした。前回分のマージで安心してぼーっとしてしまい、気が回ってませんでした。 正直、この PR に関しては isearch のコードの2重持ちということになってしまうので、手放しで「採用してほしい」とも言えません(isearch でカタカナが使えるようになるので、TUT-Code ユーザーにはメリットがあるのかもしれませんが、私にはよくわかりません)。必要性が無さそうなら、私に気兼ねせず気軽に reject してもらえればと思います。 ついでにここに書いてしまいますが、ほかにこんな修正も用意できますがいりますでしょうか。重要度が低い割に量が多いものもあるので、レビューの負担を考えると、気軽に提出してよいものか悩みます。逆に、この PR より先にレビューしていただけるものがあれば、取り急ぎ PR にしますがどうでしょうか。
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
「emacs 本体の関数の上書きを無くしたい #29」の実装の続き、input method 方式での実装です。「isearch拡張機能のadviceによる再実装 #30」の commit に続ける形の commit 群です。#30 で修正が入ったら、こちらも取り換えます。(#30 が済んでからこちらを提出してもよかったのですが、早めに全体像が見えていた方がよいかとも思い、先に提出します。これで私が予定していた修正は全てです。)
修正内容の説明
バグ修正2つ、後置部首/交ぜ書き変換の実装、前置部首/交ぜ書き変換の実装、バグ修正3つ、README修正、テスト追加、という順の9個の commit です。
いくつかのバグは、
tcode-input-methodの実行タイミングが変わることに起因します。その説明を #29 に書いておきました。以下、commit log に書ききれなかったコメントです。特に従来実装に影響が無いかについて検討します。
Handle errors in input method to prevent isearch breakage (commit 3187239)
isearch 中にエラーが起きるとわけのわからないモードになって開発がしにくいので、最初にこの修正を入れました。
エラーを起こさなければ従来実装と同じ挙動です。
Fix non-terminating kbd macro when bushu conversion occurs last (commit 4935d2b)
input-method-functionがnilを返すのはあまり望ましくないようです。代わりに、ダミーのイベントtcode-ignoreを返し、次のように何もしないコマンドignoreにバインドしました。global-mapは交換可能なので、デフォルトのglobal-mapでバインドするだけでは不十分なのでは? とも考えたのですが、global-mapを取り換えるコードを emacs ソースから探したところ、Calc や edt (古いエディタのエミュレータ) など、日本語入力をしなさそうな状況のものばかりなので、問題ないと考えました。tcode-use-input-methodがnilのときは従来通りnilを返します。Add isearch support for tcode-use-input-method (commit ef576d5)
(setq tcode-use-isearch :im)で input method 方式の isearch になるようにしました。この commit では、後置部首/交ぜ書き変換まで実装しています。isearch 中の特殊な
input-method-functionの呼び出しの事情については、コード内のコメントで説明しています。tc-bushu.el、tc-mazegaki.el に全く変更が無い点がセールスポイントです。
従来コードと比べると、ラッパーを挟んでいるだけです。
this-commandがisearch-with-input-methodでない限り従来と同じコードパスを通ります。isearch-with-input-methodが呼ばれるのは、isearch-process-search-multibyte-charactersから。isearch-process-search-multibyte-charactersが呼ばれるのはisearch-printing-charからですが、tc-is22.el でも、advice 方式でも、T-Code モードの場合はその呼び出しを通りません。Implement prefix-bushu/mazegaki conversion in isearch (commit 5fcc755)
前置部首/交ぜ書き変換の実装です。tc-bushu.el、tc-mazegaki.el の変更は、変換開始関数にラッパーを挟んだのと、終了を知らせる関数呼び出しを追加したのみです。
前者(変換開始関数のラッパー)は、
tcode--in-isearch-flagが立っていないときは何もしません。tcode--in-isearch-flagを立てるのは、前 commit のラッパーで isearch 中と判断したときですが、これは従来実装では通らないコードでした。後者(変換終了を知らせる関数)は、tcode--in-minibuffer-for-conversion-flagが立っていないときは何もしません。このフラグが立つのは、変換開始時にtcode--in-isearch-flagが立っていたときのみ通るコードです。結局、従来実装では新規コードの部分を通りません。Fix undo amalgamation of insertion and deletion (commit 6c60284)
tc-ja-alnum input method と T-Code input method でどちらも
last-commandをセットしているのですが、不要と考え削除しました。last-commandというのは、input method が呼ばれる前のコマンドで、例えばカーソル移動かも知れませんし、ファイルのセーブかも知れません。input method とは関係がありません。削除しないと、前の編集コマンドが文字挿入とみなされ、これから行なう挿入と一緒に undo されてしまいます。tc-ja-alnum input method は小さいので、内部で
last-commandを用いていないのは明らかです(同時にセットしているlast-command-eventも用いていません)。T-Code input method も調べたところで内部ではlast-commandを使ってなさそうです。last-command-eventは、内部から呼ばれるサブコマンドで使用しているので、そのままにしてあります。last-commandは、次のコマンドが始まる前にthis-commandの値で上書きされてしまうので、内部で使っていないとすると、あと参照するタイミングは何かのフックぐらいしかなさそうです。ちょうど、tc-complete.el の completion 機能がpost-command-hookを使ってその中でlast-commandを参照しているので、これとの連絡のためにlast-commandを流用していたのだと推測しています。tc-complete.el はコードが壊れていて現状動作しないので、これへの影響を考える必要は現時点ではなさそうです。この修正は、従来実装が通る部分の動作を変更しています。
(unless tcode-use-input-method ...)として従来実装への影響を無くすこともできますが、上に述べた話でつじつまが合うことから、そこまでする必要もないかと考えました。Fix handling of overriding-terminal-local-map (commit a0f2c27)
C-u 人で「人」が4つ表示されるはずが、「m」が4つ表示されてしまいます。ほかにもset-transient-mapされた状況はすべて同様の問題があります。quail のコードを真似て修正しました。これも従来実装で通る部分の修正ですが、input method 方式でこの修正に問題が無いのであれば、マイナーモード方式ではもっと安全のはずです。transient-map はもう解除されているはずですし、そもそも
tcode-self-insert-commandがコマンドとして実行中ということは、キーマップを参照するフェーズはもう済んでいるわけですから、このタイミングでキーマップの存在によって動作を変える必要性があるとは考えにくいです。Fix SPC insertion being affected by previous prefix argument (commit 9b25833)
input method 方式で
current-prefix-argが使えないという #29 で説明したそのままの問題です。if文により、従来実装では従来通りの動作です。Update README.md: add :im to the list of isearch options (commit 3c55356)
README.md に
:imの説明を追加しました。Add tests (commit 725c63c)
いくつか細かい挙動のテストを追加しました。
2実装の比較
片方だけ採用される可能性もあると思い、
:advice方式と:im方式の利点・欠点を比較してみました。:advice方式、:im方式 共通の利点isearch-lax-whitespaceぐらい?)が動作するようになった。:advice方式 の利点:advice方式 の欠点(従来のtc-is22.elから欠点ではあるが。)tcode-input-method、部首/交ぜ書き変換のコードの一部が、isearch 用に2重に実装されている。:im方式 の利点input-method-functionという、input method 実装のための正規の機能を使った実装になっている。:im方式 の欠点input-method-functionに許されている以上のふるまいをしている感がある(後置変換によるバッファ改変、前置変換のための minibuffer 使用など)。そのために「hack」的な無理をしているとも言える(post-command-hookの多用)。