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

Mac: Cmdをショートカットキーの修飾キーとして使えるようにする #594

Merged
merged 11 commits into from
Dec 17, 2021

Conversation

PickledChair
Copy link
Member

内容

ショートカットキーの設定について、Mac でも Shift とCtrl, Alt キーは修飾キーとして使えていましたが、Cmd キーは修飾キーとして使用できませんでした。これを使用できるようにします。

備考: Mac のショートカットキー操作は Undo 操作は Cmd + Z、保存操作は Cmd + S など、Cmd キーを多用します。Mac 対応において Cmd キーが使えるかどうかということは大きい問題だと考えています。

関連 Issue

ref #572

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

use_cmd_key_mac_720p.mov

その他

Windows と Linux のショートカットキーの設定や使用には影響を与えていないと思います(Ubuntu で Windows キーを押した時などの動作を確認しました)。しかし、いくつか議論の余地がある箇所がありそうです。レビュワーのご意見によっては一旦クローズして、課題解決のための Issue を開くかもしれません。

Meta キーの扱いについて

Mac の Cmd キーはブラウザ上では Meta キーとして扱われます。Meta キーは Windows や Ubuntu では通常 Windows キーです( Ubuntu では Super キーと呼ぶようです)。現状、VOICEVOX では Windows や Ubuntu で Meta キーを使えないようにしているため、「Mac のみが Meta キーを修飾キーとして扱う」と仮定して実装しました。そこで、vue のコンポーネントに「 key の名前として Meta が来たら Cmd に置き換える」という処理をハードコードしてしまっています。例として以下のような箇所があります:

        <template v-for="(hotkey, index) in lastRecord.split(' ')" :key="index">
          <span v-if="index !== 0"> + </span>
          <q-chip :ripple="false" color="setting-item">
            {{ hotkey === "Meta" ? "Cmd" : hotkey }}
          </q-chip>
        </template>

他 OS で今後 Meta キーをどう扱うつもりなのかによっては、これは良くなさそうな気がしているので、良い案があればご提案よろしくお願いします。

また、何らかの事情により「ショートカットキーの表示名を変える」必要があるかもしれないという話題は #547 でも触れられていました。今のところ Windows と Linux で Windows キーの表示はなされないものの、今回提出した PR はそのままだとバグのもとになるかもしれないという認識はしております。姑息的な解決策としてマージしてもらうか、根本的な解決策が見出されるまでペンディングすべきか、自分には判断がつきかねたので、意見を伺うため、一応 PR として出してみました。

OS判定コードについて

voicevox/src/store/setting.ts 内で OS 判定をする方法を色々調べたのですが、Web 系の技術に慣れていないため、UserAgent を使った方法しか探し出せませんでした(他に process.platform$q = useQuasar(); $q.platform.is.mac などが使えないか試したのですがうまくいきませんでした)。より良い方法があれば教えていただければと思います。

@madosuki
Copy link
Contributor

Quasarの方はどうしたらいいかわからないですが、process.platformはElectronではメインプロセスで実行しないとOS名が返ってこないのでbackground.tsに既に定義されているipcMainHandle群の様にprocess.platformの結果を返すハンドルを設定し、src/electron/preload.tssrc/type/preload.tssrc/type/ipc.d.tsをそれに沿うように変更すれば、storeでも実行されているOSがわかると思います。

@PickledChair
Copy link
Member Author

@madosuki ありがとうございます。参考になります!

src/store/setting.ts Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

@.madosuki さんの仰るように、electronのbackground.tsで判定して持ってくるというのが筋が通っていそうな気もしますが、まあuaでの判定でも良いかなと感じました!

@PickledChair
Copy link
Member Author

@Hiroshiba コメントありがとうございます。教えていただいた Quasar のドキュメントをよく読んだら正しい方法がわかったので、Quasar を使った OS 判定に変えます。

また、Meta キー周りはとりあえず Mac だけ Meta キーを使い他の OS では扱わない旨をコメントで残すようにしようと思います。その処置で足りない恐れがある場合はご指摘ください。

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!
実際どなたかwindowsで動作チェックして頂けると助かります。
(夜にいらっしゃらなければ夜に試そうかなと思います)

@PickledChair
Copy link
Member Author

メニューの表示で Meta を Cmd に置き換えるのを忘れていました。ここにも置換処理を入れます。

menubar_meta_bug

@madosuki
Copy link
Contributor

Windowsでショートカットキー周り試しました。
0.9.3からの挙動の変化は、キー設定時の画面でMetaが表示されることがあったがこちらのPRのコードでは表示されなくなったということぐらいですが、ショートカットキーとして使えたわけではないですし、意図通りだと思うので特に問題ないと思います。

@ghost
Copy link

ghost commented Dec 16, 2021

キー設定時の画面でMetaが表示されることがあった

ショートカットキーとして使えたわけではない

これについて気になったので確認してみたところ、 Ctrl + Meta 等で実際にショートカットキーとして動作するようです。
使っている人は流石にいなさそうですが…。

@PickledChair
Copy link
Member Author

@madosuki @seiga-mametani

動作確認ありがとうございます。実際には挙動に変化があるんですね。調べたところ、確かに今までの仕様では Shift or Ctrl or Alt の後に Windows キーを押すと Meta が表示され、ショートカットキーとして使えそうです。意図された仕様ではないとは思うのですが、今回の変化( Windows で Windows キーを VOICEVOX のショートカットキーとして使えなくなる)は許容されそうでしょうか? @Hiroshiba

@Hiroshiba
Copy link
Member

OSごとに表示(CmdなのかWinキーなのか)する機構を実装しちゃうと解決ですが、プルリクエストの範囲外という感じでしょうか。

windowsキーが使えなくなるのは一旦許容してしまっても良いと思いますが、過去にwinをショートカットキーに設定していた人の挙動がちょっと問題になるかもと感じました。
・・・ここのmac判定をなくしてしまって、winも動くんだけどCmdとして表示されちゃうようにしちゃいますか!

https://github.com/VOICEVOX/voicevox/pull/594/files#diff-1cf76189b82408c4654ebfe27c57b43bc279261c5642b3f4bcc0595a83393917R223-R227

でwinでもCmdと表示されるというissueを作れば、まあ開発も既存ユーザーも未来のユーザーもちょっとずつ痛み分けという形にできるかなと思いました。

@PickledChair
Copy link
Member Author

PickledChair commented Dec 16, 2021

Windows 版ユーザーにとっては不可解な仕様になりますが、「あまりショートカットキーとして Windows キーを使っている人はいなさそうであり、この変更を入れてしまって様子見」ということでしょうか(仮に Windows キーを使っていた人がいたとしても、表示が Cmd になってしまう以外は一応互換性が保たれる)。了解しました。PR がマージされたら早いうちに Issue を立てようと思います。

@PickledChair
Copy link
Member Author

PickledChair commented Dec 16, 2021

指摘された箇所のOS判定を取り除くと、それはそれで過去の動作と異なるようになる([Shift|Ctrl|Alt] + Meta が使えなくなる)ことがわかりました。代わりに、以下の変更(修飾キーに Meta を加えるかどうかについてさらにOS判定する)を加えると今回想定している動作になりました。

https://github.com/VOICEVOX/voicevox/pull/594/files#diff-1cf76189b82408c4654ebfe27c57b43bc279261c5642b3f4bcc0595a83393917L224-R235

@Hiroshiba
Copy link
Member

[Shift|Ctrl|Alt] + Meta が使えなくなる

なるほどです。
過去に登録したショートカットキーはたぶん使えるというのと、実装が煩雑になってしまうのとで、この互換性は切っちゃって良いかなと感じました。
切っちゃいましょう・・・!

これまでは(おそらく意図せず)[Ctrl|Shift|Alt] + Meta をショートカットキーとして
登録・使用することが可能となっていたが、この変更で不可能になる
@PickledChair
Copy link
Member Author

過去に登録したショートカットキーはたぶん使えるというのと、実装が煩雑になってしまうのとで、この互換性は切っちゃって良いかなと感じました。
切っちゃいましょう・・・!

わかりました。互換性を切るという方針で、該当部分のコードをPR当初のものに戻しました。

@Hiroshiba Hiroshiba self-requested a review December 16, 2021 16:41
src/store/setting.ts Outdated Show resolved Hide resolved
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!!
変更が小さくてわかりやすくて良いなと思いました。

win・linuxでMetaを良い感じに表示するというissue作成もお願いします!

Copy link
Member

@y-chan y-chan left a comment

Choose a reason for hiding this comment

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

LGTMです!

要議論な点は、Issue化して別途進めましょう...!

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.

None yet

4 participants