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

Add: N番目のキャラクターを選択するホットキーを追加 #1896

Closed
wants to merge 28 commits into from

Conversation

tsym77yoshi
Copy link
Contributor

@tsym77yoshi tsym77yoshi commented Mar 3, 2024

内容

複数のキーを一つのホットキー動作に割り当てる機能した後、
N番目のキャラクターを選択するホットキーを追加。
その後、読み取り専用にしないように修正。
?+1が1キャラ目、?+2が2キャラ目、...?+0が10キャラ目(デフォルトでは未設定)

関連 Issue

close #1187

その他

  • ブラウザ版で確認したので複数選択時の挙動が不明
  • Mac/Linuxでの動作不明
  • 設定のマイグレーションが何だかわからなかった

より良くしようとすればできそうな点

  • HotkeySettingDialogのduplicatedHotkeyのコードの改善?
  • argumentKey!=undefinedのものをHotkeySettingDialogのreadonlyHotkeyKeysに自動で追加できるようにする(読み込みの順番の問題?で自分では上手くできなかった)
  • ↑ではなくそもそもキー割り当てを任意にできるように変更するした
  • 余計なデータまで保存する必要がある のを改善する構造を変更した

もっとその他

一緒に解決されたしていなかった。一部発生するバグを解決するのにここで作られるであろう関数が必要 ref #1947
(shiftkey, Arrowを使うとバグが発生する)

@tsym77yoshi tsym77yoshi requested a review from a team as a code owner March 3, 2024 11:49
@tsym77yoshi tsym77yoshi requested review from y-chan and removed request for a team March 3, 2024 11:49
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へとミスの修正
@tsym77yoshi
Copy link
Contributor Author

読み取り専用にすると以前のキーと被った時に困ると気づいたので修正します

内容
1.HotkeySettingDialog内
- duplicatedHotkey(重複したキー)を複数へ
- ショートカットキー入力画面で+(数字)が出るように(lastArgumentKeyText)
- ショートカットキー入力画面でキー入力した時にargumentKeyがあるなら変化させる
- changeHotkeySettingsでargumentKeyも登録されるように
- duplicatedHotkeyを探索するときの処理時、自分が+(数字)等の時の探索方法の変化
- キーをデフォルトへ戻すときにduplicatedkeyの関数を利用
hotkeyPlugin内
- getArgumentKeyCombinationTextをより使いやすく
preload/0.13.json内
- 「N番目のキャラクターを選択」のデフォルトを未設定に
BindignKey -> BindingKey
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/components/CharacterButton.vue Outdated Show resolved Hide resolved
src/type/preload.ts Outdated Show resolved Hide resolved
src/components/Dialog/HotkeySettingDialog.vue Outdated Show resolved Hide resolved
src/components/CharacterButton.vue Outdated Show resolved Hide resolved
src/components/Dialog/HotkeySettingDialog.vue Outdated Show resolved Hide resolved
src/components/Dialog/HotkeySettingDialog.vue Outdated Show resolved Hide resolved
src/components/Dialog/HotkeySettingDialog.vue Outdated Show resolved Hide resolved
src/components/Dialog/HotkeySettingDialog.vue Outdated Show resolved Hide resolved
src/components/Dialog/HotkeySettingDialog.vue Outdated Show resolved Hide resolved
src/components/Dialog/HotkeySettingDialog.vue Outdated Show resolved Hide resolved
@tsym77yoshi
Copy link
Contributor Author

tsym77yoshi commented Mar 22, 2024

レビューありがとうございます!

それと追加の点で、不要なデータを保存させてしまうのを解決できていません🙇
argumenKeyを分離して保存するということもできそうなのですがref:#547 などのことを考える(気にしなくてもいいかもしれませんが)と同じところに置きたく、良い実装方法が思いついていません...何かあれば!
[次のcommit]分離してもそんなに問題を感じなかったのでやっぱり分けました

tsym77yoshi and others added 3 commits March 25, 2024 01:48
argumentKey(ホットキーの+[数字])に関する仕様変更

分離してみたところ意外とコードが複雑にならず、やっぱり分離するのが一番良さそうかなと思って変更しました
[変更後]
新たにHotkeyArgumentKeySettingsを追加
defaultHotkeySettingsの仕様はpull request前と同じに戻す
↑
[変更前]
defaultHotkeySettingsの仕様を変更
shiftkeyを使うときにエラーがでるのを修正
ついでに close VOICEVOX#1947
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.

変更プルリクエストを作って送信しました! tsym77yoshi#1

それとは関係なく、以下は自分用のメモです 🙏

  • Vuexのselect_voiceアクションだけじゃなく、select_speakerアクションも作り、そのアクション内でデフォルトスタイルを取ってくるようにするissueを作る

characterSelectShortcutの関数渡しを一段回減らす
shift+数字の問題を解決するcommitが、世に存在する色々なキーボードが存在しているのに対応していないことに気が付き元に戻す。
文字コードで操作していたがkeyCodeでやるべき
@tsym77yoshi
Copy link
Contributor Author

tsym77yoshi commented Mar 29, 2024

ありがとうございます!mergeしました!

それとshiftと矢印に関連するバグが見つかりました。原因が1947と同じで、その上でこの自分のpullrequestのコードも追加で修正しないといけなさそうです!

追記:次のcommitで解決しました!

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.

ArgumentKeyのより分かりやすい呼び方がないかちょっと考えてみました!
なかなか良いのが思いつかないのですが、RoleKey、日本語で役割キーという造語はどうでしょうか・・・?

処理が全体的にArgumentKey未割り当てのキーボードショートカットでうまく動作しないような気配を感じたのでコメントさせていただきました!
なんか自分の認識は間違ってる気がするので遠慮なくご指摘ください 🙇

src/components/CharacterButton.vue Outdated Show resolved Hide resolved
src/components/Dialog/HotkeySettingDialog.vue Outdated Show resolved Hide resolved
src/components/Dialog/HotkeySettingDialog.vue Outdated Show resolved Hide resolved
src/components/Dialog/HotkeySettingDialog.vue Outdated Show resolved Hide resolved
src/components/Dialog/HotkeySettingDialog.vue Show resolved Hide resolved
src/components/Dialog/HotkeySettingDialog.vue Outdated Show resolved Hide resolved
src/components/Talk/TalkEditor.vue Show resolved Hide resolved
src/plugins/hotkeyPlugin.ts Outdated Show resolved Hide resolved
src/plugins/hotkeyPlugin.ts Outdated Show resolved Hide resolved
src/type/preload.ts Outdated Show resolved Hide resolved
@tsym77yoshi
Copy link
Contributor Author

tsym77yoshi commented Apr 9, 2024

レビューありがとうございます!

ArgumentKey未割り当てのキーボードショートカットの問題はgetRoleKeyCombinationの処理によって解決しています。この処理では、ArgumentKeyが割り当てられていない場合にundefinedを代入し、それにより[undefined]が返されるようになっています。説明を書いていなかったのでわかりにくい状態でした 🙇

上のcommit群は順番通りでだいたい一つ一つになっていると思います。以下は例外です

  • fix: コメントを正しいものへで変更したコメントを次のcommitで消してしまったがあまり問題がなかったのでそのままに
  • refactor: 重複キーを探す処理をわかりやすくでのミスがその次のcommitで修正
  • refactor: 変数名の変更BindingKey->bindingKeyでのフォーマット整形忘れが次のcommitに

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.

整理ありがとうございます!!
かなり読みやすくなってきたかなと!!

すみません、コメントを全部書いた後に気づいたのですが、もしかしてもしかしたら、1番目のキャラクターにするホットキーを数字の1以外に割り当てたくなるのもあるかもと今更思いつきました・・・。
今は入力不可かもですが、例えばF1とか・・・。

他に上下左右キーも、ゲームの移動キーとかでよくあるWASDキーを割り当てたいとか、左右は[``]に割り当てたいとかもありそうかもなと。。。
その場合そもそもの実装方針が大きく変わるかもなので、先にちょっと意見を伺えると。。 🙇 🙇

Comment on lines +153 to +160
<span v-if="lastActionRoleKey !== undefined"> + </span>
<QChip
v-if="lastActionRoleKey !== undefined"
:ripple="false"
color="surface"
>
{{ getRoleKeyCombinationText(lastActionRoleKey) }}
</QChip>
Copy link
Member

Choose a reason for hiding this comment

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

ここのコードは上にあるコードと同じはずなので、一緒にする方針で実装しちゃいたいですね!

lastActionRoleKeylastRecordが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>(() =>
Copy link
Member

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);
Copy link
Member

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に直してあげるとよいのかもと思いました!

Comment on lines +302 to +310
// 役割キーがある場合は修飾キーのみにする
if (lastActionRoleKey.value != undefined) {
recordedCombo = HotkeyCombination(
recordedCombo
.split(" ")
.filter((item) => ["Ctrl", "Shift", "Alt", "Meta"].includes(item))
.join(" ")
);
}
Copy link
Member

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 = (
Copy link
Member

Choose a reason for hiding this comment

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

この関数は表示のためだけに利用される(保存に使われない)ので、.vue側に書くのが良いかなと!

Comment on lines +460 to +462
const hotkeyRoleKeySchema = z
.enum(["Numbers", "VerticalArrows", "Arrows"])
.optional();
Copy link
Member

Choose a reason for hiding this comment

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

hotkeyRoleKeyがoptionalなのは違和感がありました!
undefinedじゃないroleKeyの型が必要なときに不便かなと。
(実際hotkeyRoleKeySettingSchemaroleKeyにundefinedが入ることは無いはず。

Comment on lines +51 to +54
{
"action": "N番目のキャラクターを選択",
"combination": ""
},
Copy link
Member

Choose a reason for hiding this comment

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

このファイルは過去の設定なので変えちゃうとまずいかなと!

Comment on lines +465 to +469
const hotkeyRoleKeySettingSchema = z.object({
action: hotkeyActionNameSchema,
roleKey: hotkeyRoleKeySchema,
});
type HotkeyRoleKeySettingType = z.infer<typeof hotkeyRoleKeySettingSchema>;
Copy link
Member

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をつけています)

Comment on lines +100 to +102
action: "N番目のキャラクターを選択",
combination: HotkeyCombination(""),
// HotkeyRoleKeySettingsに[数字]などは存在
Copy link
Member

Choose a reason for hiding this comment

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

このコメントはちょっと意図が伝わらないかもです・・・!
書くなら・・・例えばこうとか?

Suggested change
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(""),
Copy link
Member

Choose a reason for hiding this comment

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

あ、最初からctrl+数字を登録することって可能そうでしょうか 👀

@tsym77yoshi
Copy link
Contributor Author

レビューありがとうございます!

コメントを全部書いた後に気づいたのですが、もしかしてもしかしたら、1番目のキャラクターにするホットキーを数字の1以外に割り当てたくなるのもあるかもと今更思いつきました・・・。
今は入力不可かもですが、例えばF1とか・・・。

他に上下左右キーも、ゲームの移動キーとかでよくあるWASDキーを割り当てたいとか、左右は[``]に割り当てたいとかもありそうかもなと。。。
その場合そもそもの実装方針が大きく変わるかもなので、先にちょっと意見を伺えると。。 🙇 🙇

確かにあると良さそうですね!

- 案1 置き換えるものを保存 既存のものはそのままにし、置き換えるホットキーのactionとroleKeyKey(各役割キー(例:"0","1","Up"))とcombinationの組み合わせを保存
  • 実装の流れ
  1. 保存する型を設定
  2. preload.tsで保存・呼び出し関数の設定
  3. 設定を起動時に呼び出しHotkeyPluginに渡す
  4. 設定からキーコンボを探す関数の追加(name, keyが引数、返り値はkeyComboか"")
  5. 役割キーを探す処理を上記の関数に統合(?)
  6. ホットキー設定画面のUI作成
  7. 同画面での設定の保存処理
  8. 同画面でのキー重複時の処理変更(検索・削除)
  9. キーバインド処理に追加
  10. キー入力時の関数の引数をhotkeyPlugin側で処理
  • メリット
    既存のコードをあまり変えない
  • デメリット
    新しく保存する列ができてしまう。例えば数字からF1に変更するときに変更する欄の数が多くなる。Find関数を酷使することになる?
- 案2 全てを保存 HotkeySettingTypeのcombinationを、combination(既存)かcombinations(例:[{"roleKeyKey":"Up", combination:"Ctrl W"}...])のどちらかを選択できるものにする
  • 実装の流れ
  1. 保存する型を設定
  2. 色々な処理を変更(どれだけ必要かが不明なので大雑把にまとめました)
  3. ホットキー設定画面のUI作成
  4. 同画面でのキー重複時の処理変更(検索・削除)
  5. キーバインド処理に追加
  6. キー入力時の関数の引数をhotkeyPlugin側で処理
  • メリット
    combinationに書かれているものがそのままキー名になるのでわかりやすい
    最終的なコードの量は最小...?
  • デメリット
    基準となるキーがないのでホットキー設定画面で一工夫必要?
    他に影響を及ぼす
- 案3 保存の効率を求めた案 例えば矢印からWASDに変えることと、左キーだけ無効にするということだけを考えて、actionとkeyType(例:"WASD","Arrows")、actionとroleKeyKey(例:"Left"が登録されていれば左を無効化)を保存する
  • メリット
    保存の効率がいい
  • デメリット
    自由度が低い。保存する種類が2種類も増え、それを変換する作業もあるので、非常に複雑な処理になる予感がする。

一応案を三つ考えてみました!自分的には1か2ですね!(2の方がいいのかな..?コードを見た経験が少ないのでどっちの方が良さそうか自分には最終的に確定はできませんでした)他にもなにか良さそうなものがあれば!

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 19, 2024

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

頂いた案はRoleKeyとユーザーが設定したキーをどちらもなにかしらの方法で持っておく、といった感じでしょうか。
どの方法が良いか難しいですが、やるなら僕も2みたいな流れかなと感じました!
1の置き換える方針の場合、置き換える前のキーが変わった(デフォルトのキーが変わった)ときにややこしくなりそうだなーと。

他にもなにか良さそうなものがあれば!

ちょっと整理してみました!

前提として「N番目のキャラを選択」というようにひとまとめにできると考えていたんですよね。
でもN番目それぞれに対して別々にショートカットキーを設定したくなるかも、というユースケースが出てきたのかなと。

となると「N番目のキャラを選択」みたいにショートカットキー設定UIをひとまとめにできなさそうだなーと思いました。
であれば、いっそ「1番目のキャラを選択」~「9番目のキャラを選択」それぞれのショートカットキーを新設してしまっても良いのかもと思いました。

UX的に不便じゃないか・わかりやすいかが気になっています。
たまにVSCodeを参考にしてるのですが、数字を使うショートカットはこんな感じでした。
ものすごい量のショートカットキーがあるのですが、検索ができるのでまあまあ便利でした。
image

どういう実装が良いのかなかなか難しいのですが、どういう方針が良さそうかまた案をいただけると心強いです 🙇

@tsym77yoshi
Copy link
Contributor Author

なるほど!既に検索機能もあるのでこれで不便にはならないと思います!Blenderとかもそうなっていました!

(このissueには直接の関係はありませんが)ホットキーをトーク、ソング、共通の三種類に分けると多少分類ができて見やすくなるかもしれません。ですが種類が3種類しかないことからその価値はそこまで高くなさそうです


このpullrequestはcloseして新しいpullrequestを作る感じで大丈夫でしょうか?
それとこの実装方針でdefaultHotkeySettingsを10個連続で書くのと、defaulthotkeysettingsをletにしてfor文で追加するのではどちらが良さそうでしょうか?

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 20, 2024

とても助かります、ありがとうございます!! 🙇

ホットキーをトーク、ソング、共通の三種類に分けると多少分類ができて見やすくなるかもしれません

大別するのとても良いと思います!
いつかぜひやりたいところです!!

このpullrequestはcloseして新しいpullrequestを作る感じで大丈夫でしょうか?

もし将来的にRoleKeyがほしくなったとき、実装コードがあったほうがわかりやすそうな気がします!
なのでこちらのPRは今のコードで残しておきつつcloseだとありがたいかもです・・・!

それとこの実装方針でdefaultHotkeySettingsを10個連続で書くのと、defaulthotkeysettingsをletにしてfor文で追加するのではどちらが良さそうでしょうか?

後者のが良さそうですが、Actionがリテラル文字列になってたりで難しそうだったら前者でも良いかなと!
ちなみにletにしなくてもスプレッド構文使えば良い感じに書けるかもです。

let numberStore = [0, 1, 2];
let newNumber = 12;
numberStore = [...numberStore, newNumber]; // ...がそう

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

Successfully merging this pull request may close these issues.

キャラクター変更ショートカットキー
2 participants