change: HotkeysJSを代替 #1984
change: HotkeysJSを代替 #1984
Conversation
1. App.vueでキー入力を受け付ける 2. hotkeyPluginでキー入力に反応する 3. 以前のkeyInputされたときのif文などの判定を（190~200行目あたり）を移動 4. actions内で入力されたものと一致するか検索しあれば実行する hotkeysJsの為に書かれた部分を消す作業はしていませんが一応動作部分はこんな感じで大丈夫だと思います
1. App.vueでキー入力を受け付ける 2. hotkeyPluginでキー入力に反応する 3. 以前のkeyInputされたときのif文などの判定を（190~200行目あたり）を移動 4. actions内で入力されたものと一致するか検索しあれば実行する hotkeysJsの為に書かれた部分を消す作業はしていませんが一応動作部分はこんな感じで大丈夫だと思います
|
なるほど、すべてのキーで反応させる感じですかね！！ 入力のたびに処理が走りまくるので、TTSソフトとしては結構避けたほうが良い･･･かも･･･？ うーーーーーーーーーーーーーん判断ができない。 もし実装する機運がある場合は、まずはコールバック内の処理がだいぶ軽いことを把握するか、軽くない場合はより軽くできるか、なんとかして不要なときにコールバックが実行されない形にするか（これはかなり難しい気がする）･･････････？ |
|
割と軽いと思います。そもそもhotkeys-jsも内部でこんな（keydown毎に列挙）感じになっていると思います。 |
コメントアウトで消すのをやめる 型を書く不要な処理を削除 filterの順番を一般的に速くなりそうな順番に
… into change-hotkey
コード読んだのですが、流石に全部にコールバックが呼ばれる形には（たしか）なっておらず、１つ以上hotkeyが登録されたものだけlistenされる形だったと記憶しています。 |
そもそも特定のキーのkeydownをlistenするのは自分が知ってる限りではできないです。 |
|
@sevenc-nanashi なるほどです。そうかも。 |
|
２つのショートカットキーで、片方が片方を内包しているときの挙動ってどうなってそうでしょう･･･？ 👀 |
|
==でつないでいるので含まれるものでは反応しません、例えばCtrlとSpaceが押されていれば"Ctrl Space"という文字列が返ってくるので！ |
|
あ、あるショートカットキーを入力してるときに別のショートカットキーを通過する場合を考えてました！ 今のPRの方向性で難しい機能でニーズありそうなのは、修飾キー以外の２つのキー同時押しあたりくらい…？ 自前実装アリな気がしてきました！！ |
キーを押す関数(HotkeyManager.keyInput)があるのでそれを実行して関数の実行回数を検査 keyとcodeの両方を書いているが将来的に統一が決まったら削除する
|
|
計測ありがとうございます！！問題なさそうですね･･･！！ issueの方での懸念事項洗い出しが完了したらプルリクエストをレビューさせていただこうと思います！！
分からないのですが、多分ないと思います･･･！ Line 286 in ded825f ここからblameを使って遡って行って、ずっとそうだったら問題ないと思います！！ |
hotkeyPlugin.ts内のEditor型はEditorType型内の要素数が増えてきたら型定義をより一般な形に変更
src/plugins/hotkeyPlugin.ts
Outdated
| @@ -33,7 +33,8 @@ export const useHotkeyManager = () => { | |||
| return { hotkeyManager, registerHotkeyWithCleanup }; | |||
| }; | |||
|
|
|||
| type Editor = "talk" | "song"; | |||
| // FIXME: EditorType型内の要素数が増えてきたら型定義をより一般な形に変更 | |||
There was a problem hiding this comment.
このFIXMEがいつになっても解決されないかもしれないですね！
（要素数が増えない可能性がありそう）
意図を明確にするか、なくても良いかも？
There was a problem hiding this comment.
Dialogや簡単編集モードのようなEditorを追加する時のことを想定していました（4つのeditorでさえ15個になってしまうので…）
しかし、それらの中でも使わない組み合わせが大半なので型定義を将来的にも変える必要は別になさそうな気がしました
（HotkeyAction内のeditorをeditor = EditorType | EditorType[];にして、登録時に展開するというのも考えたのですがこれはあまりよくないですかね？）
There was a problem hiding this comment.
あ、なるほどです！！たしかに増えてくるとどうにかしていかないとですね。
まあパターンはそんなに増えないかもなので、一旦コメントはなくても良いかもと思いました！
editorのとこの型を変えるの良いと思います！！
EditorType | EditorType[]とするのもまあアリですが、関数を使う側の手間はあまり変わらないのでシンプルにeditors: EditorType[]が良いかもです･･･！
でも結構今のPRも要素盛り盛りなので、一旦マージ目指せると嬉しいです！！
| // MetaキーはCommandキーとして扱う | ||
| // NOTE: hotkeys-jsにはWinキーが無く、Commandキーとして扱われている | ||
| // NOTE: Metaキーは以前採用していたmousetrapがそうだった名残り | ||
| // NOTE: hotkeys-jsでは方向キーのarrowプレフィックスが不要 | ||
| const bindingKey = combination | ||
| .toLowerCase() | ||
| .split(" ") | ||
| .map((key) => (key === "meta" ? "command" : key)) | ||
| .map((key) => key.replace("arrow", "")) | ||
| .join("+"); | ||
| return bindingKey as BindingKey; | ||
| // 順番が違うものも一致させるために並べ替え | ||
| return combination.split(" ").sort().join(" ") as BindingKey; | ||
| }; |
There was a problem hiding this comment.
.sort()で並べ替える処理をしています!
There was a problem hiding this comment.
とても良さそうに思いました！！！
実装ありがとうございます！！
テスト周りに関してもコメントしました！
ちょっと気になったのですが、既存のテストは「ショートカットキーが実行されるか」ではなく「ショートカットキーを登録できるか」のテストが主なように見えました。
「登録されたか」と「実行できるか」のテストを分けてあげるとクールかもですね！
今のままだと前者のテストができませんが、HotkeyManagerにbindされている物のリストを返すメソッドを生やしてあげれば可能になると思います。
もしご興味あれば･･･！（そのままでも、あるいは後で実装でももちろん大丈夫です！）
|
レビューありがとうございます！ コメント-commit対応一覧> 一応addとremoveを対にしておけると嬉しそうです！
c5c0e89
7bd034a
317c672
|
内容
hotkeysJsの部分をコメントアウト
hotkeysJsの為に書かれた部分を消す作業はしていませんが一応動作の中心部分はこんな感じで大丈夫だと思います
関連 Issue
ref #1947 ：一応解決することはできていますが保存形式に関する議論は解決していないのでrefとしました
ref #1964 ：同じissueですがこちらとは衝突しません
close #2024 ：3b0ed98 です
その他
combinationToBindingKey関数を利用して一致検索しているので.code、.keyどちらで保存する仕様にしても動作します。shift+"となっていても（表示は変ですが）実行できます（未実装の複合キーが困りますが）。