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

アプリのUIサイズを変更するショートカットの追加 #2380

Merged
merged 9 commits into from
Dec 3, 2024

Conversation

kebin628
Copy link
Contributor

@kebin628 kebin628 commented Nov 27, 2024

内容

以前途中まで行っていた拡大縮小のショートカットPRを最新版で再実装しました。
見様見真似の実装なので、おかしな所あればご指摘お願いします。
実装アプローチは以前のモノと同様です。

以前のもの → #1557

  • ボイス&ソングの表示メニューに拡大・縮小・拡大率のリセット追加
  • 設定→キー割当に拡大(Ctrl+)、縮小(Ctrl-)、拡大率のリセット(CtrlAlt0)を追加。

【議題】拡大率リセットのショートカットについて

拡大率のリセットは以前はCtrl+0でしたが、現在は10番目のキャラクターが占有している為、暫定的に初期値をCtrl+Alt+0にしています。
ちなみにChromeのズームリセットショートカットはCtrl+0で、Adobe Photoshopの場合はCtrl+Alt+0になっているそうです。

Adobe Camera Raw での初期設定のキーボードショートカット
https://helpx.adobe.com/jp/camera-raw/using/default-keyboard-shortcuts.html

一応、Ctrl+Alt+0は使われてはいるようですが、レアなようなので

  • Ctrl + Alt + 0の暫定ショートカットで良い(PRの初期実装のままでいく)
  • Ctrl + 0の10番目キャラ選択ショートカットをズームリセットに明け渡す
  • 別のショートカットのほうが良さそうであればそちらにする
  • Ctrl + - が生きているのであれば重要度は低いのでショートカット初期設定を空にしユーザー側で入れて貰う(どちらにせよメニューから操作ができる)
    については相談したほうが良いかもしれません。

このあたりのUIやショートカット決定フローについては現在どこで相談すれば大丈夫でしょうか?
一応、このあたりの話はコードを少し変えるだけでなんとかなるので、後から変える分では全然問題ありません。

よろしくお願いします。

関連 Issue

アプリ全体を縮小・拡大できるようにする by kebin628 · Pull Request #1557 · VOICEVOX/voicevox
#1557

スクリーンショット・動画など

image
image
image

その他

@kebin628
Copy link
Contributor Author

テストがうまく行ってないようなので、確認します

@kebin628
Copy link
Contributor Author

Type '{ getAppInfos(): Promise<{ name: string; version: string; }>; getTextAsset(textType: K): Promise; getAltPortInfos(): Promise<{}>; ... 36 more ...; reloadApp(): never; }' is missing the following properties from type 'Sandbox': zoomIn, zoomOut, zoomReset
Sandbox側でも関数定義未定義であれば未定義と入れないとダメって感じですかね?
image

あとは自分が変更してない部分(Unchanged files with check annotations Preview)で何か起きているようですが、コレは一体・・・

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.

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

ちょっとメンテナ視点で微調整させていただきたい項目があったのでこちらからプルリクエスト送らせていただきました!
ついでにエラーも解決しています!
もしよかったら取り込んでいただいたり参考にしていただければ 🙏

一応こちらで動作確認していますが、問題なさそうかどうかも見ていただけると助かります!

そのプルリクエストの中に含めている変更に対してのコメントを書いてみました、もしご興味あれば!

@@ -623,7 +623,22 @@ registerIpcMainHandle<IpcMainHandle>({
win.maximize();
}
},

// UIの拡大率拡大
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 117 to 132
{
type: "button",
label: "拡大",
onClick: () => {
void window.backend.zoomIn();
},
disableWhenUiLocked: false,
},
{
type: "button",
label: "縮小",
onClick: () => {
void window.backend.zoomOut();
},
disableWhenUiLocked: true,
},
Copy link
Member

Choose a reason for hiding this comment

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

sing/talkで統一のメニューは統一して書くこともできるので、その形に変えさせていただきます!

type: "button",
label: "拡大率のリセット",
onClick: () => {
void window.backend.zoomReset();
Copy link
Member

Choose a reason for hiding this comment

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

あ、あとwindow.backendを.vueとかから直接呼ぶのは避ける方針にしているのでそれも反映させていただきます!
ちなみにVuexを経由するようになっていて、そのVuexの実装がなかったからエラーが出ている感じでした!
(より正確には型定義だけされていた感じでした)

Comment on lines 451 to 453
"拡大",
"縮小",
"拡大率のリセット",
Copy link
Member

Choose a reason for hiding this comment

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

ちょっと後ろの方にさせていただきます!
(特に問題ないはずですが、まぁ追加の順序が分かりやすいようにぐらいのきもちです)

@Hiroshiba
Copy link
Member

Sandbox側でも関数定義未定義であれば未定義と入れないとダメって感じですかね?

あ、ですね! もしよかったら入れていただければ 🙏
というかまぁそこは確かブラウザ版なのですが、ブラウザ版はズーム機能不要ですね。
こういう時どうするか決めてないので、とりあえずthrow errorしていただけると!!

色々気を使ってくださってありがとうございました!!
調査もありがとうございます!

拡大率のリセットですが、おそらくショートカットキーを割り当てなくても問題ないだろうなと思いました。そんなに利用しないはずなので。
むしろctrl+alt+0を他のキーに使いたい時にちょっとまずいかもなので、一旦割り当てなしでも良いかも。
・・・もしよかったら割り当てなしにしていただけると!!
(ちなみに設定にctrl+alt+0が保存されていると思うので、1回設定を消さないと挙動チェックできないかもです)

@kebin628
Copy link
Contributor Author

@Hiroshiba
了解ですー
ちょっと明日以降のが都合がいいので、明日以降確認してマージしますね

@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Nov 29, 2024

🚀 プレビュー用ページを作成しました 🚀

更新時点でのコミットハッシュ:6726759

@kebin628
Copy link
Contributor Author

kebin628 commented Nov 29, 2024

@Hiroshiba
PR確認しました。
問題無かったのでこのPRにマージしました。

Sandboxに追加した関数定義のThrow入れました。
Lintのエラー消えてますね。

それと、拡大率のリセットショートカット定義を空にしました。

余談なのですが、全体のショートカットについてはもとから別のショートカット(Ctrl+Lとか)を設定して確認していました。

npm run electron:serve

だとどうもブラウザベース?で立ち上がり、Ctrl+-が最初から割り振られていたんですよね。(ChromeのDevツールが横に出る)
この状態だと、今回の実装なしでもCtrl+-で拡大自体はできました。
逆に、ビルド済みのボイボエディタだとできなかったですね。

なので、確認のため使ってないショートカットに割り振って動作確認していたのですが、アプリ形式?でまだ確認できていません。
これアプリベースでの動作を確認する場合、何か方法ありますか?
そっちでも確認しないとダメそうな気が今になってしていまして…

@kebin628 kebin628 marked this pull request as ready for review November 29, 2024 03:59
@kebin628 kebin628 requested a review from a team as a code owner November 29, 2024 03:59
@kebin628 kebin628 requested review from Hiroshiba and removed request for a team November 29, 2024 03:59
@Hiroshiba
Copy link
Member

一応手元の環境でビルドすることもできます!
npm run electron:buildとかで。
https://github.com/VOICEVOX/voicevox#%E3%83%93%E3%83%AB%E3%83%89

エンジンを同梱するのがちょっとややこしいと思います。
そのままビルドしたら"dist_electron\win-unpacked\VOICEVOX.exe"にビルドされると思います。
手元でビルドしてみた感じなんかエラー出ましたが。。

アプリを起動したあと、別でエンジンを立ち上げれば起動するはずです。
(エンジンの起動に失敗したという旨のエラーダイアログが表示されますが。)

あと今日本語ファイル名になってるコードが落ちる関係で、tests/ディレクトリを消さないとビルドできないかもです。


こちらでビルドも試してみました!
もしかしたらctrl +が動いてないかもでした!!

だとどうもブラウザベース?で立ち上がり、Ctrl+-が最初から割り振られていたんですよね。(ChromeのDevツールが横に出る)

↑の件ですが、開発時はデフォルトのメニューが登録されてるからかもです!

if (!isDevelopment) {
Menu.setApplicationMenu(null);
}

ここのif (!isDevelopment) {を消してあげれば実行時環境と同じになる・・・と思います。
実際にctrl +が動かなかったので・・・!!

ctrl shift ;とかなのかも)

@kebin628
Copy link
Contributor Author

kebin628 commented Dec 1, 2024

@Hiroshiba
自環境だとMenu.setApplicationMenu(null);消しても非ビルド版(ブラウザ版?)の拡大縮小は消えませんでした。
おそらく、これについてはビルドしてやった方のバージョンで確認するしかなさそうです。
幸い、ビルドからのアプリっぽい感じで動かすことはできました。

それと、+-がそれぞれビルド版で動くかについてですが、動きませんでした。
ショートカットの挙動については、どうも上の非ビルド版のショートカットのせいでややこしくなっていたようです。

また、ショートカットキーの入力でCtrl++を登録しようとしたところ、どうもそもそもCtrl++がボイボのショートカットとして登録が現状できないようで、おかしいなと思って登録場所であるhotkeyPlugin.tsの317行目にあるeventの中を見たら、ちょっと別件で気になることが出てきました。

自環境はHHKB Pro 2(USキーボード)で、PC設定もUSキーボード使用の設定です。→ https://happyhackingkb.com/jp/products/discontinued/hairetu.html#pro2
一番注目して欲しいのは「Shift=」で「+」が出る事です。

それで、ショートカット登録のコード見ると、

const eventKey = event.code.replace(/Key|Digit|Numpad/, "");
if (
/^([A-Z]|\d|Arrow(Up|Down|Left|Right)|Enter|Space|Backspace|Delete|Escape|F\d+|-|/)$/.test(
eventKey,
)
) {
recordedCombination += eventKey;
}
となっており、参照先がevent.codeです。

それで、気になったことが2点あります。

ショートカットにEqual, Minusが現状登録不可?

少なくとも、USキーボードだとCtrl+Shift+=で+を出すので、event.code参照だとif内の条件によりEqualが登録できない状態のようです。
image

また、「Ctrl+-」を出す場合、CodeがMinusで返って来るので、「-」も同様に登録不可です。
image

同様に「Ctrl+Shift+;」もCodeがSemicolonで返ってくるので登録不可です。
image

そのため、拡大縮小を入れるショートカットをベーシックに作るためには、この辺の改造が必要かもしれません。

キーボード言語設定によりCtrl++がそもそも現状だと初期設定しようがない?

自分がUSキーボード使いで、「+」を入れる場合、「Shift + =」を入れる必要があります。
それで、この場合ショートカットの初期登録がCtrl + Shift + Equalがとなります。
一方、JSキーボードだと「+」は「Shift + ;」となるので、初期設定の値が異なります。
なので、現状のもので初期ショートカットをCodeベースで作ろうとすると、もしかするとキーボードの違いに対応できないかもしれません。

以上です。

もしかすると、このショートカット類を実際にキーボードで入力される数値(code.key)ベースにしてやらないと微妙に困るシチュエーションが出るかもしれません。
ただ、これは現段階で登録部分の表面をサラッと見た状態で、実際にショートカットが認識されるところがどうなっているかは未確認なので、逆にcode.keyにすると困るところが出るかもしれません。

いずれにせよ、現状の改修だとCtrl++が不可能っぽい上にCtrl+Shift+;もダメそうなので、拡大縮小については未設定で一度回した方がいいかもしれません。
別件でIssue立てて詳しい人に全体眺めてもらったほうが良さそうです。

@kebin628
Copy link
Contributor Author

kebin628 commented Dec 2, 2024

@Hiroshiba
こんにちは。本件の方針相談です。

現状Ctrl+-の設定が難しそうで、Ctrl+Shift+Semicolonも改造が必要そうになり、検証箇所がかなり増える見込みです。
また、キーボードの種類に応じて初期ショートカットが破茶滅茶(例えばShift+SemicolonだとUSキーボードだとコロンが出る)など、色々厄介な問題を孕んでいることから、別問題にして、機能だけ投入するのがベターな感じがしています。

そのため

  • 拡大縮小で使っていたショートカットキーの設定を全て未設定状態にする(枠としては残し、各自使えるボタンで好きに登録してもらう形に現段階では変更)
  • ショートカットの件についてはIssue新たに立てる
    をした上で、マージの方針で進めてしまって大丈夫でしょうか?

@Hiroshiba
Copy link
Member

なるほどです、ありがとうございます!!
keyとcodeに関してはそういえば以前めちゃめちゃ調べたのを思い出しました。。。。

ctrl++ですが、jsキーボードなら普通に動いた…かもなのでちょっとこちらでもう一度確認します!!
とりあえずctrl +とctrl -で実装してしまうのはどうでしょう…?
(確認はマージ後に自動ビルドされるので、そこでチェックでも良いかも!)

@kebin628
Copy link
Contributor Author

kebin628 commented Dec 2, 2024

@Hiroshiba
ctrl++をアプリ(ブラウザでない)で動かせ、かつショートカット登録画面で設定ができれば特に思うことはないので、もし今のCtrl++がヒホさんの多分JSキーボードの環境で動かせるか見てもらって、それでうまく行けばいまのまま、うまくいかなければどうするか決めるのがいいと思います。

多分ですけど、ヒホさん側のPCでも、ショートカット登録画面で上述の理由からCtrl+Shift+;が登録できないはずです(スクショの画面)
image

@Hiroshiba
Copy link
Member

@kebin628
承知しました!! マージした後に自動ビルドが走るので、それで試してみたいと思います!
(多分問題なく動く・・・はず!)

ショートカットキー登録ですが、こちらでも試してみたところ確かにうまく動かさなそうでした・・・。
これかなり厄介なんですよね。。。。。。

多分ショートカットキーライブラリを@rwh/keystrokesあたりに置き換えて色々辻褄を合わせればうまくいきそうな予感があります。
これはコードとキーをちゃんと扱っていそうなライブラリでした。
ただ利用者数などを見つつ、メンテナンス性を考えると・・・・・・・・。

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

マージします!
実装ありがとうございました!!!!

@Hiroshiba Hiroshiba merged commit 7ba0863 into VOICEVOX:main Dec 3, 2024
10 checks passed
@Hiroshiba
Copy link
Member

ビルドした成果物をJSキーボードで試してみました!

@kebin628 さんの仰るとおり、ctrl++は登録もできず、初期登録されているctrl++で色々やってみても拡大できませんでした!
(ちょっと後者は個人的には予想外でした)

一応macでも挙動を確認してみたところ同じでした。(cmd+-は動く)

提案いただいた形の

ショートカットの件についてはIssue新たに立てる

で進められればと思っています!
まあメニューから設定はできるので、致命的な問題ではないと考えてます。
ショートカットキーが効かない件は、放置しても良いとは思わないですが、結構根が深いので一旦仕方ないという気持ちです。

なんかこの辺り最高のライブラリがあればよいのですが。。。。。。。。
VSCode辺りに独立して存在しないかなぁ。。。。

@kebin628
Copy link
Contributor Author

kebin628 commented Dec 4, 2024

@Hiroshiba

拡大縮小で使っていたショートカットキーの設定を全て未設定状態にする(枠としては残し、各自使えるボタンで好きに登録してもらう形に現段階では変更)

これの話決定前にMainにマージ入ったようなのですが、新たにプルリク立てて初期ショートカットの設定消す方針はどうしましょうか?
ユーザ目線最初に映ってるショートカットキー(Ctrl++, Ctrl+-)が動かないようであれば、一端消したほうがバグ味ないかなぁという感じがしてまして。

cmd+-は大丈夫だったとのことなので、Macだけ初期ショトカ残す=スクショの感じにするのがベターかな?と思っていますが、いかがでしょう?
image

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