キーバインドで必須と任意の修飾キーを設定できるようにし、Option + Enterを .enterとして扱うようにする #173

mtgto · 2024-06-12T14:38:14Z

#172 で問題となった、Option + Enterがv0.23.0からIMEで拾われるようになってしまったことへの対処として、KeyBinding.Inputに「必須の修飾キーの集合」と「押していてもよい任意の修飾キーの集合」を設定するようにします。

2024-06-15: 既存のキーバインドは、矢印キーはShiftあり、エンターはOptionありとしていますがマージする前にもう少し考えます。

y-yu

ちょっとアドホックな気もしますが、とはいえIntelliJが不便な状況になっちゃてるので直るならマージでよさそう 🙆‍♀️

macSKK/KeyBindingSet.swift

y-yu · 2024-06-12T15:57:09Z

macSKK/KeyBindingSet.swift

+            if keyBind.0.key == input.key {
+                switch input.key {
+                case .character(let c):
+                    if KeyBinding.Key.characters.contains(c) && keyBind.0.modifierFlags == input.modifierFlags {


KeyBinding.Key.characters.contains(c)はこれinput.keyのコンストラクト時 👇 にもチェックしています。

macSKK/macSKK/KeyBinding.swift

Lines 107 to 109 in 18db2f0

init(event: NSEvent) {

if let character = event.charactersIgnoringModifiers?.lowercased().first, Key.characters.contains(character) {

key = .character(character)

よって.characterのケースに進入した以上はもうこのチェックは不要ということで

Suggested change

if KeyBinding.Key.characters.contains(c) && keyBind.0.modifierFlags == input.modifierFlags {

keyBind.0.modifierFlags == input.modifierFlags {

☝️ こうしても大丈夫なような 🤔

y-yu · 2024-06-12T15:59:10Z

macSKK/KeyBindingSet.swift

+                        return keyBind.1
+                    }
+                case .code(let code):
+                    if input.modifierFlags.contains(keyBind.0.modifierFlags) {


ほほー、input.key側が.codeかどうかによってmodifierFlagsを完全一致にするかcontainsにするかどうか的なロジックですか 👀
これは 👇 こういうちゃんとした（？）modifierFlagsの取り扱いをするまでの一旦の凌ぎみたいな感じということかな 🤔

Option+Enterを.enterとして扱うためdefaultKeyBindingSettingsに追加 #172 (comment)

直感でいけるかな? とおおまかに分けたんですが、さすがに自信がないので見直します。
このPR内でv0.22.xまでのKeyBinding入力前と同じような挙動になるように修正しようかなと思っています。

「ちゃんとしたmodifierFlagsの取り扱い」をするのもそう大変ではなかったので、大きく舵切ってこのPRでやることにしました。 f9f259d
将来ユーザーがキーバインドをカスタマイズするときには必須modifierFlagsにユーザーが入力したもの、optionalFlagsはデフォルトは空でユーザーがカスタマイズ可能にするつもりです (EnterをOption押しててもありにする、みたいなカスタマイズ)。

自分もこれ実は裏でやろうとしてたんですが、なかなかoptionalFlagsをどう持つかとかがあまりうまくいかず、いったんOption + Enterだけ局所的にどうにかする方向でいいか 🤔 と思ってましたが、全体的にこのPRでやってよくなっていると思います 👍

Co-authored-by: YOSHIMURA Yuu <yyu@mental.poker>

y-yu

些細な指摘もしたけどトータルでよさそう！ 👍

y-yu · 2024-06-15T08:00:55Z

macSKK/KeyBinding.swift

-            case .code:
-                hasher.combine(modifierFlags.subtracting(.shift).rawValue)
-            }
-        }


（メモ）
この.shiftを特別扱いするhashを消すの、個人的にはすごく賛成です 👍
ディクショナリーのキーなどに暗黙的に用いられるであろう関数において.shiftに関する特別処理が記述されているというのは後々なにか問題の種になる可能性があると思ったので、optionalModifierFlags導入とKeyBindingSet.dict廃止によりこのhashが消えることは将来にわたってとてもいい判断になると確信しています。

y-yu · 2024-06-15T08:02:02Z

macSKK/KeyBinding.swift

-            } else {
-                return lhs.key == rhs.key && lhs.modifierFlags == rhs.modifierFlags
-            }
+            return lhs.key == rhs.key && lhs.modifierFlags == rhs.modifierFlags && lhs.optionalModifierFlags == rhs.optionalModifierFlags


（メモ）
同様にこの==も.shiftに関する特別処理が消えて相当自明になったと思うのでこの実装を支持します 👍

y-yu · 2024-06-15T08:07:08Z

macSKK/KeyBinding.swift

+            if let character = event.charactersIgnoringModifiers?.lowercased().first, Key.characters.contains(character) {
+                if key != .character(character) {
+                    return false
+                }
+            } else if key != .code(event.keyCode) {
+                return false
+            }


めちゃくちゃ微粒子レベルの指摘にはなりますが、else ifでやっている比較のほうがなんか単純な比較だけなので、先にそっちをやったほうが、そっちでもうreturn falseとなったら Key.characters.containsをやらなくてよくなって超微妙に効率がよかったりますかね？

Suggested change

if let character = event.charactersIgnoringModifiers?.lowercased().first, Key.characters.contains(character) {

if key != .character(character) {

return false

}

} else if key != .code(event.keyCode) {

return false

}

if key != .code(event.keyCode) {

return false

} else if let character = event.charactersIgnoringModifiers?.lowercased().first, Key.characters.contains(character) {

if key != .character(character) {

return false

}

}

このaccept、最大で登録されているキーバインドの回数だけ呼ばれるのでそれも含めて富豪的な書き方になってますね。
ちゃんとやるならNSEventを引数にするのを止めて、ここでやってるちょっとした前処理を初期化で済ませた別の構造体に入れておくかなあとか思います。

struct CurrentInput { let key: Key let modifierFlags: NSEvent.ModifierFlags init(event: NSEvent) { if let character = event.charactersIgnoringModifiers?.lowercased().first, Key.characters.contains(character) { self.key = .character(character) } ... } }

みたいな。

なるほど、そうですね 👍
もともとはNSEventからKeyBinding.Inputをインスタンシーエションしていて、KeyBindingに用いるInputというデータ構造と入力のmodifierFlagsやキーコードなどを整理しておくデータ構造は別であったほうがいいと思ってました。
今回のPRの変更では、やや副作用かもしれませんがKeyBinding.InputがNSEventからインスタンシーエションされるパスが消滅しKeyBinding.Inputが真にKeyBinding用のデータ構造になったと思います。いったんこのPRの段階では今の状態でも十分とは思いますが、将来的にはCurrentInputのようなデータ構造がKeyBinding.Inputとは別に定義されるのはとてもいいと思いました 👍

y-yu · 2024-06-15T08:13:09Z

macSKK/KeyBinding.swift

+            } else if key != .code(event.keyCode) {
+                return false
+            }
+            return modifierFlags.isSubset(of: event.modifierFlags) && modifierFlags.union(optionalModifierFlags).isSuperset(of: event.modifierFlags)


なるほど、たしかにisSubsetとisSupersetでこう表現できるのか。（メモ）

y-yu

よさそう。

mtgto · 2024-06-16T02:43:11Z

レビューありがとうございました。

mtgto added 2 commits June 12, 2024 23:36

Option+EnterのようにmodifierFlagsがついていてもEnterとして扱うようにする

4f85922

Shift + ; のような印字キーは修飾キーがキーバインドと完全一致する必要がある

4b11a7c

y-yu mentioned this pull request Jun 12, 2024

Option+Enterを.enterとして扱うためdefaultKeyBindingSettingsに追加 #172

Closed

y-yu reviewed Jun 12, 2024

View reviewed changes

mtgto and others added 3 commits June 14, 2024 21:18

KeyBindingSetで並べる順番についてコメントを追加

3136935

Co-authored-by: YOSHIMURA Yuu <yyu@mental.poker>

不要な比較を削除

89df418

KeyBinding.Inputに許容する修飾キーを設定し厳密に比較できるように修正

f9f259d

mtgto changed the title ~~Option + Enterを .enterとして扱う~~ キーバインドで必須と任意の修飾キーを設定できるようにし、Option + Enterを .enterとして扱うようにする Jun 15, 2024

y-yu approved these changes Jun 15, 2024

View reviewed changes

メソッド名のリファクタ

e3aedf6

mtgto merged commit 5c29a36 into main Jun 16, 2024
2 checks passed

mtgto deleted the option-enter branch June 16, 2024 02:42

mtgto mentioned this pull request Jun 16, 2024

関心のある修飾キー以外は存在しない扱いをする #175

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

キーバインドで必須と任意の修飾キーを設定できるようにし、Option + Enterを .enterとして扱うようにする #173

キーバインドで必須と任意の修飾キーを設定できるようにし、Option + Enterを .enterとして扱うようにする #173

mtgto commented Jun 12, 2024 •

edited

Loading

y-yu left a comment

y-yu Jun 12, 2024

y-yu Jun 12, 2024

mtgto Jun 14, 2024

mtgto Jun 15, 2024

y-yu Jun 15, 2024

y-yu left a comment

y-yu Jun 15, 2024

y-yu Jun 15, 2024

y-yu Jun 15, 2024

mtgto Jun 15, 2024

y-yu Jun 15, 2024

y-yu Jun 15, 2024

y-yu left a comment

mtgto commented Jun 16, 2024

	init(event: NSEvent) {
	if let character = event.charactersIgnoringModifiers?.lowercased().first, Key.characters.contains(character) {
	key = .character(character)

	if KeyBinding.Key.characters.contains(c) && keyBind.0.modifierFlags == input.modifierFlags {
	keyBind.0.modifierFlags == input.modifierFlags {

キーバインドで必須と任意の修飾キーを設定できるようにし、Option + Enterを .enterとして扱うようにする #173

キーバインドで必須と任意の修飾キーを設定できるようにし、Option + Enterを .enterとして扱うようにする #173

Conversation

mtgto commented Jun 12, 2024 • edited Loading

y-yu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y-yu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y-yu left a comment

Choose a reason for hiding this comment

mtgto commented Jun 16, 2024

mtgto commented Jun 12, 2024 •

edited

Loading