-
Notifications
You must be signed in to change notification settings - Fork 298
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
Add: N番目のキャラクターを選択するホットキーを追加 #1896
Conversation
N番目のキャラクターを選択するホットキーを追加 ctrl+1が1キャラ目、ctrl+2が2キャラ目、...ctrl+0が10キャラ目 ## 実装の概要 - 新しいホットキーの追加 - TalkEditor->AudioCell->CharacterButtonへと関数を呼び出していきCharacterButton内で関数を実行 ## その他 - ブラウザでやっているので複数個選択した時の挙動が不明 - Mac/Linuxでの動作が不明 - 設定のmigrationがよくわかりませんでした
ctrl+[数字]等のホットキー設定を可能に 注:ホットキーは読み取り専用(ユーザーが変更できない設定)にしてください ## 実装の概要 1.preload 新たにargumentKey型("Numbers" | "VerticalArrows" | "HorizontalArrows" | undefined)を追加(真ん中二つは不要だが将来追加するかもしれないときの為に) 2.hotkeyPlugin キーをバインド設定/解除するときにargumentKeyだけループ 一部関数追加 3.HotkeySettingDialog argumentKeyを表示させる キーが被った時に警告を出す ## この後の展開 N番目のキャラクターを選択するホットキーを追加 ## ホットキーの設定方法 - 通常のホットキー設定と同様のことを行う - preloadのdefaultHotkeySettingsでargumentKeyを設定 例)argumentKey: "Numbers", - HotkeySettingDialogでreadonlyHotkeyKeysに追加 - (e)=>{}のe.keyからキー入力を取得できる ## より良くしようとすればできそうな点 - HotkeySettingDialogのduplicatedHotkeyのコードの改善 - argumentKey!=undefinedのものをHotkeySettingDialogのreadonlyHotkeyKeysに自動で追加できるようにする(読み込みの順番の問題?) - ↑ではなくそもそもキー割り当てを任意のものに変更可能にする - 余計なデータまで保存する必要があるのを改善する
argumet -> argumentへとミスの修正
読み取り専用にすると以前のキーと被った時に困ると気づいたので修正します |
内容 1.HotkeySettingDialog内 - duplicatedHotkey(重複したキー)を複数へ - ショートカットキー入力画面で+(数字)が出るように(lastArgumentKeyText) - ショートカットキー入力画面でキー入力した時にargumentKeyがあるなら変化させる - changeHotkeySettingsでargumentKeyも登録されるように - duplicatedHotkeyを探索するときの処理時、自分が+(数字)等の時の探索方法の変化 - キーをデフォルトへ戻すときにduplicatedkeyの関数を利用 hotkeyPlugin内 - getArgumentKeyCombinationTextをより使いやすく preload/0.13.json内 - 「N番目のキャラクターを選択」のデフォルトを未設定に
293b054
to
62cbd78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
プルリクエストありがとうございます!!!
実装読ませていただきました、かなり広範囲のコードを把握しながら書いてくださったんだなと感じました!!
とても労力がかかったと思います、本当にありがとうございます!!!!
実装方針は非常に良いなと思いました!
あとは細かいところをリファクタリングだけ一緒にできればと思っています!!
もしよければよろしくお願いします・・・!!
Update HotkeySettingDialog.vue
…voicevox into characterShortCut_2
レビューありがとうございます!
|
argumentKey(ホットキーの+[数字])に関する仕様変更 分離してみたところ意外とコードが複雑にならず、やっぱり分離するのが一番良さそうかなと思って変更しました [変更後] 新たにHotkeyArgumentKeySettingsを追加 defaultHotkeySettingsの仕様はpull request前と同じに戻す ↑ [変更前] defaultHotkeySettingsの仕様を変更
shiftkeyを使うときにエラーがでるのを修正 ついでに close VOICEVOX#1947
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
変更プルリクエストを作って送信しました! tsym77yoshi#1
それとは関係なく、以下は自分用のメモです 🙏
- Vuexのselect_voiceアクションだけじゃなく、select_speakerアクションも作り、そのアクション内でデフォルトスタイルを取ってくるようにするissueを作る
characterSelectShortcutの関数渡しを一段回減らす
shift+数字の問題を解決するcommitが、世に存在する色々なキーボードが存在しているのに対応していないことに気が付き元に戻す。 文字コードで操作していたがkeyCodeでやるべき
ありがとうございます!mergeしました! それとshiftと矢印に関連するバグが見つかりました。原因が1947と同じで、その上でこの自分のpullrequestのコードも追加で修正しないといけなさそうです! 追記:次のcommitで解決しました! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ArgumentKey
のより分かりやすい呼び方がないかちょっと考えてみました!
なかなか良いのが思いつかないのですが、RoleKey
、日本語で役割キーという造語はどうでしょうか・・・?
処理が全体的にArgumentKey
未割り当てのキーボードショートカットでうまく動作しないような気配を感じたのでコメントさせていただきました!
なんか自分の認識は間違ってる気がするので遠慮なくご指摘ください 🙇
レビューありがとうございます!
上のcommit群は順番通りでだいたい一つ一つになっていると思います。以下は例外です
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
整理ありがとうございます!!
かなり読みやすくなってきたかなと!!
すみません、コメントを全部書いた後に気づいたのですが、もしかしてもしかしたら、1番目のキャラクターにするホットキーを数字の1以外に割り当てたくなるのもあるかもと今更思いつきました・・・。
今は入力不可かもですが、例えばF1とか・・・。
他に上下左右キーも、ゲームの移動キーとかでよくあるWASDキーを割り当てたいとか、左右は[``]
に割り当てたいとかもありそうかもなと。。。
その場合そもそもの実装方針が大きく変わるかもなので、先にちょっと意見を伺えると。。 🙇 🙇
<span v-if="lastActionRoleKey !== undefined"> + </span> | ||
<QChip | ||
v-if="lastActionRoleKey !== undefined" | ||
:ripple="false" | ||
color="surface" | ||
> | ||
{{ getRoleKeyCombinationText(lastActionRoleKey) }} | ||
</QChip> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここのコードは上にあるコードと同じはずなので、一緒にする方針で実装しちゃいたいですね!
lastActionRoleKey
やlastRecord
がundefinedや空欄かどうかという条件で何を表示するか分けるのではなく、表示する内容を返すcomputedを作るのはどうでしょう?
<script>
側でこんな感じの定義を実装するのをイメージしてます!
(名前とかは適当です)
/** 表示するキー */
const displayingKeys = computed(() => {
return lastActionRoleKey != undefined ? lastRecord : getRoleKeyCombinationText(lastActionRoleKey)
})
@@ -270,12 +287,27 @@ const hotkeyColumns = ref< | |||
|
|||
const lastAction = ref(""); | |||
const lastRecord = ref(HotkeyCombination("")); | |||
/** lastActionに役割キーがあれば取得 */ | |||
const lastActionRoleKey = computed<HotkeyRoleKeyType>(() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
だいぶ細かいことなんですが、この辺の型は勝手に推論してくれるので<HotkeyRoleKeyType>
は書かなくても大丈夫だったりします。
もし意図的に書いているのであれば、基本的に書かない方が見やすい気がします!
(↓のコードとかで: string
と書かないのと同じ気持ちです)
const hoge: string = "";
|
||
const recordCombination = (event: KeyboardEvent) => { | ||
if (!isHotkeyDialogOpened.value) { | ||
return; | ||
} else { | ||
const recordedCombo = eventToCombination(event); | ||
let recordedCombo = eventToCombination(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventToCombination
、ctrlだけが押されたときはもしかしたらControl
が返ってきているかも・・・?
だとしたらeventToCombination
側でCtrl
に直してあげるとよいのかもと思いました!
// 役割キーがある場合は修飾キーのみにする | ||
if (lastActionRoleKey.value != undefined) { | ||
recordedCombo = HotkeyCombination( | ||
recordedCombo | ||
.split(" ") | ||
.filter((item) => ["Ctrl", "Shift", "Alt", "Meta"].includes(item)) | ||
.join(" ") | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こっちで文字列操作してるのちょっと危ないかもです。例えばeventToCombination
が返す値がスペース区切りじゃなくなったとしても気付けないなと。
eventToCombination
側の引数に、修飾キーのみを返せるようにする引数を用意してあげると良さそうに思いました!
throw new Error(`Received unexpected HotkeyRoleKeyType`); | ||
}; | ||
|
||
export const getRoleKeyCombinationText = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この関数は表示のためだけに利用される(保存に使われない)ので、.vue側に書くのが良いかなと!
const hotkeyRoleKeySchema = z | ||
.enum(["Numbers", "VerticalArrows", "Arrows"]) | ||
.optional(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hotkeyRoleKey
がoptionalなのは違和感がありました!
undefinedじゃないroleKeyの型が必要なときに不便かなと。
(実際hotkeyRoleKeySettingSchema
のroleKey
にundefinedが入ることは無いはず。
{ | ||
"action": "N番目のキャラクターを選択", | ||
"combination": "" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このファイルは過去の設定なので変えちゃうとまずいかなと!
const hotkeyRoleKeySettingSchema = z.object({ | ||
action: hotkeyActionNameSchema, | ||
roleKey: hotkeyRoleKeySchema, | ||
}); | ||
type HotkeyRoleKeySettingType = z.infer<typeof hotkeyRoleKeySettingSchema>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ちょっとややこしいのですが、このスキーマ+型とHotkeyRoleKeySettings
は全部hotkeyPlugin.ts
に移動しちゃうときれいだと思います!
というのも、スキーマを定義してるのは保存のためなのですが、いろいろ整理された結果この辺は保存されることがなくなったと思うので・・・!!
hotkeyPlugin.ts
に移動するとschemaの定義が不要になり、型と定数の定義で済むのでとてもシンプルになると思います。
そしてリストじゃなくRecord型にできるのでとてもシンプルになるはず!
名前はSettingsだと他の名前と衝突してややこしいのでRecordに変えると、例えばこんな感じ?
type RoleKey = "Numbers" | "VerticalArrows" | "Arrows";
const hotkeyRoleKeyRecord: Partial<
Record<HotkeyActionNameType, RoleKey>
> = {
N番目のキャラクターを選択: "Numbers",
};
(HotkeyActionNameType
の一部のアクションだけにしかvalueがないのでPartial
をつけています)
action: "N番目のキャラクターを選択", | ||
combination: HotkeyCombination(""), | ||
// HotkeyRoleKeySettingsに[数字]などは存在 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このコメントはちょっと意図が伝わらないかもです・・・!
書くなら・・・例えばこうとか?
action: "N番目のキャラクターを選択", | |
combination: HotkeyCombination(""), | |
// HotkeyRoleKeySettingsに[数字]などは存在 | |
action: "N番目のキャラクターを選択", | |
combination: HotkeyCombination(""), // + [数字] |
@@ -96,6 +96,11 @@ export const defaultHotkeySettings: HotkeySettingType[] = [ | |||
action: "長さ欄を表示", | |||
combination: HotkeyCombination("3"), | |||
}, | |||
{ | |||
action: "N番目のキャラクターを選択", | |||
combination: HotkeyCombination(""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あ、最初からctrl+数字を登録することって可能そうでしょうか 👀
レビューありがとうございます!
確かにあると良さそうですね! - 案1 置き換えるものを保存既存のものはそのままにし、置き換えるホットキーのactionとroleKeyKey(各役割キー(例:"0","1","Up"))とcombinationの組み合わせを保存
- 案2 全てを保存HotkeySettingTypeのcombinationを、combination(既存)かcombinations(例:[{"roleKeyKey":"Up", combination:"Ctrl W"}...])のどちらかを選択できるものにする
- 案3 保存の効率を求めた案例えば矢印からWASDに変えることと、左キーだけ無効にするということだけを考えて、actionとkeyType(例:"WASD","Arrows")、actionとroleKeyKey(例:"Left"が登録されていれば左を無効化)を保存する
一応案を三つ考えてみました!自分的には1か2ですね!(2の方がいいのかな..?コードを見た経験が少ないのでどっちの方が良さそうか自分には最終的に確定はできませんでした)他にもなにか良さそうなものがあれば! |
なるほど!既に検索機能もあるのでこれで不便にはならないと思います!Blenderとかもそうなっていました! (このissueには直接の関係はありませんが)ホットキーをトーク、ソング、共通の三種類に分けると多少分類ができて見やすくなるかもしれません。ですが種類が3種類しかないことからその価値はそこまで高くなさそうです このpullrequestはcloseして新しいpullrequestを作る感じで大丈夫でしょうか? |
とても助かります、ありがとうございます!! 🙇
大別するのとても良いと思います!
もし将来的にRoleKeyがほしくなったとき、実装コードがあったほうがわかりやすそうな気がします!
後者のが良さそうですが、Actionがリテラル文字列になってたりで難しそうだったら前者でも良いかなと! let numberStore = [0, 1, 2];
let newNumber = 12;
numberStore = [...numberStore, newNumber]; // ...がそう |
内容
複数のキーを一つのホットキー動作に割り当てる機能した後、
N番目のキャラクターを選択するホットキーを追加。
その後、読み取り専用にしないように修正。
?+1が1キャラ目、?+2が2キャラ目、...?+0が10キャラ目(デフォルトでは未設定)
関連 Issue
close #1187
その他
より良くしようとすればできそうな点
argumentKey!=undefinedのものをHotkeySettingDialogのreadonlyHotkeyKeysに自動で追加できるようにする(読み込みの順番の問題?で自分では上手くできなかった)するした余計なデータまで保存する必要がある構造を変更した?のを改善するもっとその他
一緒に解決されたしていなかった。一部発生するバグを解決するのにここで作られるであろう関数が必要 ref #1947(shiftkey, Arrowを使うとバグが発生する)