Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

キー割り当てでShift+数字を設定すると期待動作をしない #1947

Open
3 tasks
tsym77yoshi opened this issue Mar 22, 2024 · 20 comments
Open
3 tasks
Labels

Comments

@tsym77yoshi
Copy link
Contributor

不具合の内容

キー割り当てでshift+[1~9の数字]を入力するとshift+["!"~")"]となる。
またその状態では同じキーを押しても動作しない。

現象・ログ

[HotkeyManager] Bind: shift+! to アクセント欄を表示 in talk

再現手順

キー割り当てで適当なものにshift+[1~9のいずれか]を設定する

期待動作

キー割り当てでshift+数字と表示する
shift+数字で反応するようにする

VOICEVOXのバージョン

0.17.1

OSの種類/ディストリ/バージョン

  • Windows
  • macOS
  • Linux
tsym77yoshi added a commit to tsym77yoshi/voicevox that referenced this issue Mar 25, 2024
shiftkeyを使うときにエラーがでるのを修正
ついでに close VOICEVOX#1947
@Hiroshiba
Copy link
Member

Hiroshiba commented Mar 26, 2024

issue作成ありがとうございます! 今まで気づきませんでした・・・。

解決策はちょっとわからないのですが、原因をとりあえず調べてみました。

shift+なにかで、押されたキーの方ではなく入力される文字側が記録されるのは、おそらくショートカットキー登録の時にevent.keyを参照しているからだと思われます。

if (event.key === " ") {
recordedCombination += "Space";
} else {
if (["Control", "Shift", "Alt", "Meta"].includes(event.key)) {
recordedCombination = recordedCombination.slice(0, -1);
} else {
recordedCombination +=
event.key.length > 1 ? event.key : event.key.toUpperCase();
}
}
return HotkeyCombination(recordedCombination);

event.keyではなくevent.codeの方を使うと押されているのが何かが記録できそうです。
https://developer.mozilla.org/ja/docs/Web/API/KeyboardEvent/code

ただこの場合、押されているキーが1だとすると、event.codeはDigit1というものになります。
Digit1とショートカットキー(ホットキー)用のライブラリとして使っているhotkeys-jsでは扱えないので、変換するか、Digit1のまま扱えるようにするか、何か解決策が必要そうでした。

もしさらに調べた方や知っている方がいたらコメント募集中です!
@sevenc-nanashi さんや @Patchethium さんとかは何かご存知だったり・・・?

@tsym77yoshi
Copy link
Contributor Author

あとこれに似た件なので追加するのですが、矢印キーもevent.keyではArrowUpなのですがHotkeyJSではUpとするべきなので、ホットキーが効かないということが起きています(こっちはreplaceでいけると思います)

@Hiroshiba
Copy link
Member

ほんとですね!!

詳しく調べてたのですが、とりあえず記録時はevent.codeを使って、表示や記録はその文字列からkey|digit|numpad|arrowを抜けば良さそうに感じました!
react-hotkeys-hookそうなっていそうでした・・・!

@Hiroshiba
Copy link
Member

Hiroshiba commented Mar 29, 2024

ちなみに以前使ってたMousetrap内では今は非推奨のevent.whichを使っていて、event.keyに近い挙動でした。
一方で今使ってるhotkeysjsは非推奨なevent.keyCodeを使っていました。

なのでもしかしたら過去のバージョンで登録した!とかは普通にショートカットキーとして使えていたけどhotkeysjsに変えた時点から使えなくなってるかも。
現象として面白かったので共有です。 @sevenc-nanashi @Patchethium


保存するのはevent.codeが良いのかevent.keyが良いのかなかなか微妙なとこですねぇ。
まあhotkeysjs.keyに未対応なので今は.code使うしかないのですが、理想的にはどっちなんだろうなーと。

Githubのショートカットキーみたいに?に割り当てたい人もいるだろうし(キーボードによっては?shift+/ではなくshift+8らしい)、shift+1に割り当てたい人もいるだろうしで、答えは無い気がしますねぇ 😇

@tsym77yoshi
Copy link
Contributor Author

上のpullrequestの感じでしょうか(@がBracketLeftになったりと全部に対応するのは難しそうなのでできそうなものだけ受け付ける形で書きました)!

ほんとですね!!

詳しく調べてたのですが、とりあえず記録時はevent.codeを使って、表示や記録はその文字列からkey|digit|numpad|arrowを抜けば良さそうに感じました! react-hotkeys-hookそうなっていそうでした・・・!

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 3, 2024

プルリクエストありがとうございます!!!
ちょっと今すごく渋滞しているのでいつ見れるかわからないのですが、見させていただきます!!

@がBracketLeftになったりと

KeyboardLayoutMapというのを使うと、codeっぽいのからキーボードのレイアウトを知りつつ入力される文字がどうなるのかわかるかもでした!!
chromeのコンソールあたりから↓を入力するとどんな感じか見えると思います。

await keyboard.getLayoutMap().then((m) => Array.from(m.entries()))

ちなみに僕の環境ですとこんな感じでした。

[
  ['KeyE', 'e'],
  ['KeyD', 'd'],
  ['KeyU', 'u'],
  ['Minus', '-'],
  ['KeyH', 'h'],
  ['KeyZ', 'z'],
  ['Equal', '^'],
  ['KeyP', 'p'],
  ['Semicolon', ';'],
  ['BracketRight', '['],
  ['Slash', '/'],
  ['BracketLeft', '@'],
  ['KeyL', 'l'],
  ['Digit8', '8'],
  ['KeyW', 'w'],
  ['KeyS', 's'],
  ['Digit5', '5'],
  ['Digit9', '9'],
  ['KeyO', 'o'],
  ['Period', '.'],
  ['Digit6', '6'],
  ['KeyV', 'v'],
  ['Digit3', '3'],
  ['Backquote', '半角/全角'],
  ['KeyG', 'g'],
  ['KeyJ', 'j'],
  ['KeyQ', 'q'],
  ['Digit1', '1'],
  ['KeyT', 't'],
  ['KeyY', 'y'],
  ['Quote', ':'],
  ['Backslash', ']'],
  ['KeyK', 'k'],
  ['KeyF', 'f'],
  ['KeyI', 'i'],
  ['KeyR', 'r'],
  ['KeyX', 'x'],
  ['KeyA', 'a'],
  ['Digit2', '2'],
  ['Digit7', '7'],
  ['KeyM', 'm'],
  ['Digit4', '4'],
  ['IntlYen', '¥'],
  ['Digit0', '0'],
  ['KeyN', 'n'],
  ['KeyB', 'b'],
  ['IntlRo', '\\'],
  ['KeyC', 'c'],
  ['Comma', ','],
]

↑にないキーがたくさんありそう。テンキー入力とか。

ただそもそもhotkeys-jsで@を登録してももしかしたら使えないかも・・・?
@sevenc-nanashi さんなにかご存知だったりしてませんか 👀 )

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 3, 2024

とりあえず現状のキーボードショートカットに関してまとめるとこうでしょうか。

  • 以前の仕様
    • 修飾キー+event.keyを保存(shift+1shift+!として記録)
    • mousetrapライブラリで、event.keyCodeを使ってショートカットキー判定
    • これだと日本語キーボードの@が使えないはずだけど、なぜか使える
  • 今(0.18.0)の仕様
    • event.keyを保存(shift+1shift+!として記録)
    • hotkeys-jsライブラリで、event.keyCodeを使ってショートカットキー判定
      • shift押下時は英字キーボードで対応するkeyっぽい値として扱うっぽい?
    • 日本語キーの@などの一部キーは非対応
  • 今のバグっぽい挙動まとめ
    • shift+1が登録できない
      • shift+!になってしまうため
      • これはhotkeys-jsに関係なくmousetrapでもそう
    • @が登録できない
      • hotkeys-jsライブラリが@キーの場所(BracketLeft)に対応していないため
    • テンキーで1などを操作できなくなった?

ちなみにVSCodeを調べるとこうでした。

その他メモ

理想をまとめると、たぶんこう

  • 保存は.code
    • 過去のコードに合わせるために一部の.code.key寄りに置き換えるのはあり
    • デフォルトで用意しているショートカットキーは.keyでも良いかも、そのときの記法はVSCodeに合わせると良さそう
  • 表示はKeyboardLayoutMap内にあればそれ
    • なければ.keyが良いけど、難しければ.codeそのままでも
  • 仕様ライブラリはKeystrokes > mousetrap > hotkeys-js ?
    • Keystrokesは良いライブラリな匂いはあるけどメンテされるかは不明

@tsym77yoshi
Copy link
Contributor Author

tsym77yoshi commented Apr 3, 2024

  1. 保存について
    独自の記法にすると後から見返したら分かりにくいコードになりそうなのでどちらかに統一した方がいいと思います
    デフォルトで用意しているショートカットキーも通常の保存と統一した方がコードを書くときに楽かなと思いました!(たぶんそんなに変わったキーを入れないことを考えると/特にホットキー周りで初心者向けタグが付いていることが多いと感じたので分かりやすさが大事だと思います)

  2. 表示について
    いいと思いました!もし表示が同じだけれど保存されているものが違ったときに確認できるように、hoverしたら保存されるものも見れるみたいな仕様もあるといいかもしれないです!

  3. ライブラリについて
    Keystrokes、便利そうですね!
    あとjavascriptの仕様変更があった際にはHotkeySettingDialogも書き換えなければならないことを考えると、ライブラリを使わずに独自でコードを書いてもよさそうな気もしました!(テキスト入力欄かどうかもevent.target.tagNameで簡単に取得できそうです)
    ※独自でコードを書くなら過去と合わせるために.keyで保存した方がいいと思います(一致検索なら"shift+!"でも一致させられるので)

  • 追記 取り消し部について
    .codeに統一したら過去のものが使えなくなるし、.keyにするとキャラクター変更ショートカットキー #1187 の数字が困るしで悩みどころなのでこのようには言えないなと思い取り消し線にしました

@tsym77yoshi
Copy link
Contributor Author

eventToCombination(e)と保存されているcombinationが一致した時に関数を実行するという実装で、ホットキーを動作させることができそうです!

@Hiroshiba
Copy link
Member

コメントありがとうございます!!

draftプルリク見させていただきました、とてもシンプルだなと感じました!
テキストを入力するたび毎回コールバックが実行されるため、重さがどんなもんかがだいぶ気になっていまいました。

テキスト入力がほとんど無い場合はもう自作にするのが楽な気がします。
そうじゃないので、さてKeystrokes使うのとどっちが良いかなって感じでしょうか。。。

・・・・・・・・・・どっちが良いですかねぇ 😇 😇 😇
自作する場合の検証やテストを書くことなどを考えると、まあKeystrokesなのかなぁ。。。

@tsym77yoshi
Copy link
Contributor Author

悩ましいです...

検証やテストは大変そうなのでその面ではkeystrokesの方が良さそうです

ですが、draftの方式の方が反応しないというバグは起こさないようにできます。
記録しているものをライブラリの形式に変換するときに発生する動作をしないというバグ(現在の@キーなど)は、登録と検証でどちらも同じ関数を実行しているので完全になくなります

またdraftの方では、絞り込むときにテキスト欄で有効かという最も絞りやすい物から絞り込めるので(修正しました)理論上は速くできるはずです

@Hiroshiba
Copy link
Member

Hiroshiba commented May 1, 2024

なるほどです!
結局処理速度が大丈夫かどうかと、あとテスト書けばって感じなのかなと思いました!
考慮しなくちゃいけなそうな点

  • ブラウザで動くか(テスト書けばOK)
  • 各OSで大丈夫か(chromiumが吸収してくれるから流石に大丈夫な気がする)
  • electronで動くか(↓が動けば大丈夫な気がする)
  • electronのショートカットキーと被っているとき大丈夫か(macのcmd+Cとか)
  • 1キーストローク辺りコールバックが呼ばれる回数は1回か
  • 長押ししたときに異常動作しないか
  • 実はライブラリ側がカバーしていた領域とかが無いか(hotkeysjsとkeystrokesのドキュメントを一通り見れば良さそう)

@tsym77yoshi
Copy link
Contributor Author

  • ブラウザで動くか / 各OSで大丈夫か / electronで動くか / electronのショートカットキーと被っているとき大丈夫か
    現在のHotkeySettingDialogueでもaddEventListenerを利用しているので動くと思います
    npm run browser:serveと、windowsのnpm run electron:serveでは実行が確認できました
    また、macのcmd+cはわかりません(e.preventDefaultが呼ばれるので大丈夫なのかもしれません)
  • 1キーストローク辺りコールバックが呼ばれる回数は1回か
    testで関数が実行される回数を元に検証しているのでtestが通れば大丈夫です
  • 長押ししたときに異常動作しないか
    キー入力欄で長押しした時aaaaaaaaaaとなるのと同じように、長押ししているだけで複数回呼び出されます
    これは現状と同じです
  • 実はライブラリ側がカバーしていた領域とかが無いか
    hotkeysjsのREADMEを一通り見たところ特になさそうでした

@Hiroshiba
Copy link
Member

macのcmd+cはわかりません

手元のmacでキー登録画面で試してみた感じ、cmd+cなどelectron側に設定されてそうなやつも反応してそうでした!
electron側のショートカットキーに先にeventを奪われるんだと思ってたのですが、そうじゃないっぽいですね・・・!
(この辺りに近い話かもということでメモ #675 (comment)

hotkeysjsのREADMEを一通り見たところ特になさそうでした

確認ありがとうございます!!

できれば実装まである程度さらった方が安心だろうなと思いました。
なんでこんなに慎重になってるかというと、頑張って実装していってリリースした後に問題が発覚したときに相当回復に時間がかかってしまうためです 🙇
だったら今のうちに調べておいた方が絶対いいという経験に基づいています 🙇


1つ可能性としては、ショートカットに関係なく押しっぱなしにしておきたいキーが無いか想像しておくと良いかも・・・?
スクロールキーの代わりに下矢印を押す場合とかですが・・・・でも矢印キーを押しながらショートカットキー入力したいことはなさそう・・・・・・?

あと作るライブラリの仕様なのですが、多分同じショートカットキーに2つ以上の操作を割り当てられるようにしておくと将来の変更に強いかもと思いました!
もちろん実際の UI やアプリは一旦そうなってなくてもいいですが。


まだちょっと不安なのでもしよかったら意見いただけると。。 @sevenc-nanashi
問題になりそうなところとかって思いつかれたりしませんか・・・?

@sevenc-nanashi
Copy link
Member

大丈夫だと思います。

(長押しで連続実行されるの防げないかな…って思ったけどCtrl-Zは需要がありそうだし残したほうがいいかな、って思ったりした)

@tsym77yoshi
Copy link
Contributor Author

できれば実装まである程度さらった方が安心だろうなと思いました。

確認してみました!

  • Bind等ほぼすべての機能について
    hotkeyPlugin内で完結しています
  • element関連について
    HotkeysJsの処理を切っており、hotkeyPlugin内で完結しています
  • 特別なキー処理をするについて
    • commandKey(キーコードが複数存在するとのこと?)
      全て"Meta "に変換しているので問題ないです
    • Ctrl Alt Shift Meta
      デフォルトのホットキー設定で順番を間違えると動かなくなる問題が発生するので変更を加えます※1
    • 特殊キー
      変換しないので大丈夫です
  • Scopeについて
    "all"というスコープがデフォルトで設定されており、現在は使用されていませんが関連するissue: どのエディターでも実行されるショートカットアクションを登録できる関数を作る #2024
    ("talk&song"みたいに&区切りにするように実装しようと思います)※2

1つ可能性としては、ショートカットに関係なく押しっぱなしにしておきたいキーが無いか想像しておくと良いかも・・・?
スクロールキーの代わりに下矢印を押す場合とかですが・・・・でも矢印キーを押しながらショートカットキー入力したいことはなさそう・・・・・・?

一応Numlockが想像できましたが(自宅にあるテンキーが多分そうなっている)、ctrl, alt, shift, meta以外は同時押しに関係しないので大丈夫そうです!


あと作るライブラリの仕様なのですが、多分同じショートカットキーに2つ以上の操作を割り当てられるようにしておくと将来の変更に強いかもと思いました!

実装します!※3


(その他)

  • 特別なキー入力について
    マウスにキーを割り当てて使用する使用例があるみたいです(参考: PC Watch記事)。Windows, X-Mouse-Controlではv0.19.1でも代替したものでも動作確認が取れましたが、logicoolのマウス等、マウスの機能としてついているものは不明です。
  • ダイアログを開いている間も一部のホットキーが動作している?
    ver0.19.1ではダイアログを開いている最中にも"アクセント欄を表示"等、一部ホットキーが動作しているものがありそうです※4

※1~4は右のpullRequest内で追加しようと思います #1984

@Hiroshiba
Copy link
Member

確認してみました!

VOICEVOX側の実装の確認ありがとうございます!!
確かに大丈夫そうかどうか不安がありましたが薄らぎました・・・!!

意図していたのはhotkeyjs側・mousetrap側(あと可能ならkeystrokeも)の実装の確認でした!
将来何か問題が起こるかどうかを想像しなくちゃいけなくて、先行しているライブラリの実装を見て何か特殊なことをしないといけないかどうかを確認した方が良いなと。
といってもそんな深く追っかける必要はなく、不思議なことをやっている部分がないか実装をざっと眺めてみる感じかなと思ってます!
お手数おかけしますがもしお願いできるならかなり助かります 🙇 🙇

特別なキー入力について
Windows, X-Mouse-Controlではv0.19.1でも代替したものでも動作確認が取れましたが、

マウスにキー入力を割り当てる、なるほどです!
ちなみに「動作確認が取れた」というのは、ツイートにある通り以前と同様に動作しないことが確認できたということか、逆に新しく実装することで動作することが確認できたのか、どちらでしょうか? 👀

ダイアログを開いている間も一部のホットキーが動作している?

UI_LOCK中(ダイアログを開いている時や再生中など)でも動かせる系の操作は動くようになっちゃってますね。。
ダイアログを開いている間はscopeをtalk/singから別のに変えてあげると良いのかも・・・?(実装するにしてもプルリク分けたほうが良さそう感)

@tsym77yoshi
Copy link
Contributor Author

将来何か問題が起こるかどうかを想像しなくちゃいけなくて、先行しているライブラリの実装を見て何か特殊なことをしないといけないかどうかを確認した方が良いなと。

なるほど!わかりました!

確認結果です

Moustrap

  • preventDefault/stopPropagationをreturnValue/cancelBubbleへと変更する関数
    この後者二つはJavaScriptで前者二つに置き換えられているのですが、たぶんmousetrap内の以前のコードと整合性をとるために前者のものへ変換しているコードで、必要ありませんでした

  • _resetSequenceTimer関数
    時間を利用している関数で気になりましたが、Sequenceがないので関係ない?

HotkeysJs

気になったところは前に挙げたので全部です

KeyStroke

  • onUnmounted
    キーストロークを途中で消すということを想定していそうです。流石にnodejs終了時にeventListenerは自動で破棄されると思うので関係なさそうですが...

  • sequenceTimeout
    keyStrokeでも時間関連を書いていました。"If we are advancing to the next sequence but the timeout has passed then we reset."とのことですが、時間が経ったら(デフォルトだと1000msみたいです)ホットキーを無効化するというのは、keydownでとっている今の仕様を考えると関係なさそうです

今はキーを押した瞬間にショートカットが動作していますが、離した瞬間に動作するようにすれば、押した後に間違えたと思って長押しすればキャンセルができるという仕様にできそうです

ちなみに「動作確認が取れた」というのは、ツイートにある通り以前と同様に動作しないことが確認できたということか、逆に新しく実装することで動作することが確認できたのか、どちらでしょうか? 👀

すみません、どちらでもホットキーが動作することが確認できました 🙇 。ツイートの動作しなかったというのの原因はわかりませんでした

ダイアログを開いている間はscopeをtalk/singから別のに変えてあげると良いのかも・・・?(実装するにしてもプルリク分けたほうが良さそう感)

scopeを切り替えた方が良さそうですね!
一応scopeではない方法の実装ですf9fc14c
q-dialog__innerというclassを持っているものを弾くという仕様は、以前メニューでホットキーを動作させないようにするのにq-itemクラスであるか否かで判定していたのでそれに倣った形です。vue側の変更で気づかないうちに動かなくなる可能性があるという欠点がある実装ではあります)


(今まで書いていなかったことに気が付いたこと)
メニュー項目ではショートカットキーを無効化というコメントアウトがあったのですが、以前のコードではメニュー項目でもホットキーが動作していたので、動作しないようにしました(上のリンクのcommitの"menu"の方です・q-itemクラスがあるかで無効化していたのですがメニュー項目でq-itemを使わなくなっていたために無効化されていませんでした)

@Hiroshiba
Copy link
Member

確認結果です

おーーー!! ありがとうございます!!!

mousetrapのpreventDefaultできる機能なるほどです!
まあバブリングをキャンセルするかどうかはライブラリーに登録する時に引数で明示的に指定するようにもできるはずで、明示的に指定する方が仕様的に分かりやすいかもですね!!

KeyStrokeは順番にキー操作していった場合のショートカットキー入力に対応してるんですよね。そのための関数2つなのかなと。
順番ショートカットキーが実装できるのは魅力的かもですが、現状まあなくても良さそうなのでスルーに賛成です!

大丈夫な自信がついてきました、ありがとうございます!

今はキーを押した瞬間にショートカットが動作していますが、離した瞬間に動作するようにすれば、押した後に間違えたと思って長押しすればキャンセルができるという仕様にできそうです

なるほどです。
よくある仕様はキー入力した瞬間だと思うのと、だいたいの操作はcancelかundoが可能だと思うので、押した瞬間が良いのかなと思いました!

どちらでもホットキーが動作することが確認できました

( 📝 マウスでショートカットキー入力するやつの続きコメント)
なるほどです! だったらまあ良さそうですね!

ダイアログを開いている間はscopeをtalk/singから別のに変えてあげると良いのかも・・・?(実装するにしてもプルリク分けたほうが良さそう感)

scopeを切り替えた方が良さそうですね!
一応scopeではない方法の実装ですf9fc14c
q-dialog__innerというclassを持っているものを弾くという仕様は、以前メニューでホットキーを動作させないようにするのにq-itemクラスであるか否かで判定していたのでそれに倣った形です。vue側の変更で気づかないうちに動かなくなる可能性があるという欠点がある実装ではあります)

なるほどです!
ベストなのはscopeだとは思いますが、実装がちょっと大変なので、クラス名で判定するようにしつつFIXMEコメントを残しておくとかが一旦安定なのかなと思いました!

上のリンクのcommitの"menu"の方です・q-itemクラスがあるかで無効化していたのですがメニュー項目でq-itemを使わなくなっていたために無効化されていませんでした

なるほどです、気づいてくださってありがとうございます!
実際に問題が起きているということですねぇ。。なるほどです!

@Hiroshiba
Copy link
Member

@tsym77yoshi さん、調査ありがとうございました!!

ショートカットキーは自前実装しても良いと思いました・・・!!
プルリクエストの方見させていただきます!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants