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.

Sign up for GitHub

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

Jump to bottom

FIX: ホットキーでshift+数字を登録時のエラーをなくす #1964

Merged
merged 7 commits into from
May 20, 2024
Merged

FIX: ホットキーでshift+数字を登録時のエラーをなくす #1964

merged 7 commits into from
May 20, 2024

Conversation

tsym77yoshi
Copy link
Contributor

@tsym77yoshi tsym77yoshi commented Mar 29, 2024 •
edited
Loading

内容

event.code式に書き換え
.がPeriodだったりと様々な対応しきれないものがあるので対応しているもの以外は弾くようにした

関連 Issue

ref #1947

@tsym77yoshi
FIX: ホットキーでshift+数字を登録時のエラーをなくす
b3e46ee 
event.code式に書き換え
.がPeriodだったりと様々な対応しきれないものがあるので対応しているもの以外は弾くようにした
@tsym77yoshi tsym77yoshi requested a review from a team as a code owner March 29, 2024 16:15
@tsym77yoshi tsym77yoshi requested review from y-chan and removed request for a team March 29, 2024 16:15
Hiroshiba
Hiroshiba reviewed Apr 3, 2024
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

プルリクエストありがとうございます！！

良い感じそうに思いました！！！ややこしいのにありがとうございます 🙇

保存時に.codeを.keyっぽくするべきか、保存は.codeにして登録時に.keyっぽくするかどっちが良いかの相談コメントを書きました。
もしこのあたりに頭を使うのが苦痛じゃなければお力お借りしたく･･･ 🙇

src/plugins/hotkeyPlugin.ts Outdated
// event.codeから保存する形へと変換
let eventKey = event.code.replace(/Key|Digit|Numpad|Arrow/, "");
eventKey = eventKey.length > 1 ? eventKey : eventKey.toUpperCase();
// 英字 数字 上下左右 Enter Space Backspace Delete Escape F1~のみ認める
Copy link
Member

@Hiroshiba Hiroshiba Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

この方針とても良いなと思いました！hotkeys-jsが対応してないんですよね。
https://github.com/jaywcjlove/hotkeys-js/blob/a595c55587bb71ef251c5717549854e9c73c2b8f/src/var.js#L4

追加で「hotkeys-jsが対応してないから」的なコメントに追加してあげると文脈わかりやすいかもと思いました！

たぶん-や@や/や\はわりと使われるかもなので、いつか対応してあげたいかもですね。
この中でたぶん-と/はKeyboardLayoutMapで変換したのをhotkeys-jsに使えるのですが、\と@はUSキーボードとズレてるので使えない気がしますね･･･ 😇

src/plugins/hotkeyPlugin.ts Outdated Show resolved Hide resolved
@tsym77yoshi tsym77yoshi mentioned this pull request Apr 7, 2024
Open
This was referenced May 4, 2024
Merged
Open
@tsym77yoshi tsym77yoshi requested a review from a team as a code owner May 5, 2024 15:39
@tsym77yoshi tsym77yoshi requested review from Hiroshiba and removed request for a team May 5, 2024 15:39
Hiroshiba
Hiroshiba reviewed May 11, 2024
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

すみません遅くなりました！！

ちょっとだいぶ細かい点なのですがコメントいくつかさせていただきました！！

src/plugins/hotkeyPlugin.ts Outdated Show resolved Hide resolved
src/plugins/hotkeyPlugin.ts Show resolved Hide resolved
src/plugins/hotkeyPlugin.ts Outdated Show resolved Hide resolved
src/plugins/hotkeyPlugin.ts Outdated Show resolved Hide resolved
src/plugins/hotkeyPlugin.ts Outdated Show resolved Hide resolved
@tsym77yoshi
Copy link
Contributor Author

よく考えたら#1984 にする場合、わざわざ弾く必要がなさそうなので実装が変わりそうです

  1. 保存について
    1. 現状(v0.19.1)から変更しない
      Shift+数字等がデフォルトキー設定として設定できないということ以外は全く問題ありません。issueの問題も解決しています（登録が"Shift !"のようになりますが反応するときも"Shift !"であるかで判定するので解決します）
    2. .code形式に全て切り替える
      以前数字として登録していたものを全てDigit(テンキーではない方)に書き換えることになりそうです（不可逆的変更）。またdefaultHotkeySettingsも書き換えます。
    3. 一部だけ残す
      分かりにくいのであまりやりたくないです。不可逆的変更を避けながらi.の問題点も解決します
  2. 反応について （もし何もない状態であればi.の方が良さそうですが以前とのつながりを考えるとii.です）
    1. 数字とテンキーを分ける
      以前数字キーとして登録していたものがDigit1とNumpad1（テンキーか否か）で分かれるので以前の物が効かなくなり、混乱が生じます
    2. 分けない
      テンキーとそうでない方の両方からホットキーを動作させたいという場合があまり思い浮かばないですがi.の問題点を解決します

個人的には[1-i] or [1-iiと2-i]がいいと思っています

Hiroshiba
Hiroshiba approved these changes May 19, 2024
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM！！

色々やり取り本当にありがとうございます！！
ショートカットキー周りはまだ色々ありますが、もしよかったら実装していけるととても嬉しいです！！！！！
（めちゃくちゃ助かっています！！！）

@Hiroshiba
Copy link
Member

@tsym77yoshi

よく考えたら#1984 にする場合、わざわざ弾く必要がなさそうなので実装が変わりそうです

  1. 保存について
    1. 現状(v0.19.1)から変更しない
    2. .code形式に全て切り替える
    3. 一部だけ残す
  2. 反応について （もし何もない状態であればi.の方が良さそうですが以前とのつながりを考えるとii.です）
    1. 数字とテンキーを分ける
    2. 分けない

個人的には[1-i] or [1-iiと2-i]がいいと思っています

こちらも条件分けありがとうございます！！！！

破壊的変更になってしまいますが、[1-iiと2-i]が良さそうに感じています。
一意に決まるので実装が一番わかりやすいなと。

理想は将来的に、[1-iiと2-i]に追加で、.codeと.keyどちらも指定できる形だと感じてます。
keystrokesは文字の前に@をつけたら.codeとして、そうじゃないなら.keyとして判定する感じでした。
ただまぁ.keyのが嬉しいことがほとんどないと思うので、とりあえず.codeで全部実装する形が良さそう･･･！！

ほんとにしばらくは1``2``3をNumpadに対応させたいぐらいくらいかなと。
あと将来的にはヘルプ検索がすぐできる?キーとか･･･？

@Hiroshiba
Copy link
Member

とりあえずいただいたプルリクエストを実装するのは問題なさそうになので、マージしたいと思います！！
何か懸念点とかもしあったらリリースまでに変更すれば大丈夫なので、いつも言っていただければ！！

@Hiroshiba Hiroshiba merged commit 403b44a into VOICEVOX:main May 20, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hiroshiba Hiroshiba Hiroshiba approved these changes

@y-chan y-chan Awaiting requested review from y-chan y-chan was automatically assigned from VOICEVOX/maintainer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tsym77yoshi @Hiroshiba