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

フルスクリーンモードを追加(#2251) #2273

Merged
merged 4 commits into from
Dec 3, 2024

Conversation

terapotan
Copy link
Contributor

@terapotan terapotan commented Sep 27, 2024

内容

フルスクリーンモードを追加。F11キーもしくは、メニュータブの<表示>-><全画面表示/ウィンドウ表示切替>から、全画面表示とウィンドウ表示を切り替えられるように修正を行った。

関連 Issue

ref #2251

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

2024-09-27.142127.mp4

@terapotan terapotan requested a review from a team as a code owner September 27, 2024 05:31
@terapotan terapotan requested review from Hiroshiba and removed request for a team September 27, 2024 05:31
@terapotan terapotan changed the title フルクリーンモードを追加(#2251) フルスクリーンモードを追加(#2251) Sep 27, 2024
@terapotan
Copy link
Contributor Author

@Hiroshiba
Mac版でショートカットキーが正常に動作するかは検証していません(手持ちにMacがなく……)。
その点だけご留意ください。

Copy link
Member

@sevenc-nanashi sevenc-nanashi 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/Menu/MenuBar/MenuBar.vue Show resolved Hide resolved
@@ -754,7 +754,13 @@ registerIpcMainHandle<IpcMainHandle>({
win.maximize();
}
},

TOGGLE_FULLSCREENMODE: () => {
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
TOGGLE_FULLSCREENMODE: () => {
TOGGLE_FULLSCREEN: () => {

の方がいいかも?(これを変えるなら他のところも変える必要があるはず)

Copy link
Member

Choose a reason for hiding this comment

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

たしかにFULLSCREENMODE一単語はほんのちょっぴり違和感あるかもですね!
TOGGLE_FULLSCREEN_MODEという手もありそう。

@Hiroshiba
Copy link
Member

Hiroshiba commented Sep 27, 2024

@sevenc-nanashi
レビューありがとうございます!! 助かります!!

具体的には:フルスクリーンでも最大化解除のアイコンにして、それを押してもフルスクリーンが解除されてもいいと思います。

なるほどですね!!
chromeとVSCodeをwindowsで実際にフルスクリーンにしてみたんですが、どっちも最大化や最小化のアイコンの表示がなくなってそうでした。
(×ボタンも消えたけどそれはやりすぎな気がする)

どうするのが正解かわからないですね・・・。
・・・・・とりあえず非表示がいいかも?(自信少しなし)

ちなみにフルスクリーン状態かどうかを取得するには、state.isFullscreenが使えそうでした!
https://github.com/terapotan/voicevox/blob/0118d0014ebe8844eb8987970fcb2059386bc550/src/components/Menu/MenuBar/MenuBar.vue#L82-L83

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です!! プルリクエストありがとうございます!!

せっかくなのでもしよかったら @sevenc-nanashi さんに指摘いただいた箇所の変更お願いできれば・・・!
分からなかったり時間なかったら遠慮なく言ってください!
こちらでちょいとこちらでやらさせていただこうと思います!

@@ -466,6 +471,7 @@ export const hotkeyActionNameSchema = z.enum([
"元に戻す",
"やり直す",
"新規プロジェクト",
"全画面表示/ウィンドウ表示切り替え",
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
"全画面表示/ウィンドウ表示切り替え",
"全画面表示の切り替え",

(slackが「フルスクリーン表示の切り替え」だったので真似してみました)

@sevenc-nanashi sevenc-nanashi linked an issue Sep 27, 2024 that may be closed by this pull request
@terapotan
Copy link
Contributor Author

@sevenc-nanashi レビューありがとうございます!! 助かります!!

具体的には:フルスクリーンでも最大化解除のアイコンにして、それを押してもフルスクリーンが解除されてもいいと思います。

なるほどですね!! chromeとVSCodeをwindowsで実際にフルスクリーンにしてみたんですが、どっちも最大化や最小化のアイコンの表示がなくなってそうでした。 (×ボタンも消えたけどそれはやりすぎな気がする)

どうするのが正解かわからないですね・・・。 ・・・・・とりあえず非表示がいいかも?(自信少しなし)

ちなみにフルスクリーン状態かどうかを取得するには、state.isFullscreenが使えそうでした! https://github.com/terapotan/voicevox/blob/0118d0014ebe8844eb8987970fcb2059386bc550/src/components/Menu/MenuBar/MenuBar.vue#L82-L83

@Hiroshiba @sevenc-nanashi
FireFoxとMicrosoft edgeの場合、通常時はメニューバー全非表示、画面の上の方にマウスをホバーさせたときだけメニュバーが現れて、そうなったら最大化・最小化ボタンでフルスクリーンモードを解除できる仕様でした。

個人的にはフルスクリーンモードになったらメニュバー自体が非表示になるのがしっくりくるのですが、正直どちらがいいのかは私も分からないですね……

実装的なお話をさせて頂くと、@sevenc-nanashi さんの案であればすぐに実装できるかと思います。逆に最大化・最小化ボタンを非表示にしたり、メニューバーを非表示にする案については実装の目途が立っていない状況です(試しにsetMenuBarVisibilityやsetMinimizable、setMaximizableを使って非表示にしようと試みたのですがうまく行かず……)。

@sevenc-nanashi さんの案で進めてしまいたいと思っているのですが、どうでしょうか?

@sevenc-nanashi
Copy link
Member

個人的にはフルスクリーンモードになったらメニュバー自体が非表示になるのがしっくりくるのですが、正直どちらがいいのかは私も分からないですね……

メニューバーは出しちゃっていいと思います。

ブラウザとかだと、フルスクリーンは「ページの内容に集中」みたいな意味合いでツールバーが消えてる(ツールバーはページの内容に影響しない)と思うのですが、ボイボだと「画面を大きくする」以外にないと思うんですよね(自分が思いつく限りだと)。
で、ボイボはメニューバーがないと困る場面の方が多い(書き出しとか)ので、残した方がいいと思います。

ちなみに、メニューバーを隠すのはMenuBarコンポーネントにv-ifをつけて切り替えればできると思います。(もしかしたら多少レイアウトが崩れるかもしれませんが…)

@Hiroshiba
Copy link
Member

一般的なガイドラインに合わせるのが良さそうなので、OS側のガイドラインを調べてみました。

Windows側は」フルスクリーンに関するガイドラインがない気がしますね・・・。
macOS的にはメニューバーは隠すべきみたいな方針でしたが、今の実装でも実はOS 標準のメニューバーは隠れてるので、問題ない気もしました。

ちなみにVSCodeも複雑なメニューバーを持っているのですが、フルスクリーンにするとこんな感じになりました。
メニューバーはあり続けるけど、一部のメニューアイテムは残るという正直ちょっとよくわからない挙動でした!
before↓
image
after↓
image

なので一般的な方針はなさそうなのでVOICEVOXとしてどうしたいかで決めると良さそう感!

僕は2人ともの意見どちらにも賛成で、大事だからアクセスできるべきだし、フルスクリーンの記事に従ってメニューを隠すのもありだと思います。
凝ったフルスクリーンだと、マウスカーソル上に持って行ったり下に持って行ったりするとメニューが現れるやつもあるがあると思います。多分これが一番いい!

2番目は、大事な機能なのでアクセスは可能にしておく形(つまりメニューバー全てを非表示にはしない形)かな~と思ってます。
何より手があたったりして間違えてF11を押してしまうと、戻せなくなりそうだな~と。

このプルリクエストの方針としては、一旦今のメニューバー全体表示される形で着地させるというのはどうでしょうか?
より良くすることもできそうですが、1つのプルリクエストに機能を継ぎ足すよりも、複数のプルリクエストに分けて機能を足していきたいなーと・・・!
(あとそっちの方がお互いやりやすいと思い。)

あと最大化ボタンとかをv-ifで隠すのは実装こんな感じです。もしご興味あれば!(放置でもOKです!)

@Hiroshiba
Copy link
Member

@terapotan  お疲れ様です!
もし困ってるところとかあったらかなりお気軽に何でも聞いていただければ!!
(時間取れなくて困ってるとかであればこちらとしては大丈夫です! 🙏 )

@terapotan
Copy link
Contributor Author

@Hiroshiba
返信遅れて申し訳ないです!
時間取れず進められてませんでした……
諸事情で今月いっぱいPCに触れないので、しばらくタスク進められなさそうです……

実装方針は固まっていて、実装しようと思えばすぐに出来る状況です。

@Hiroshiba
Copy link
Member

いえいえ!! もしよければいつでも!!

では一応、このプルリクエストは、もし他の方が引き継ぎたかったら自由について OK という感じにさせてください! 🙏
もちろん @terapotan さんのお帰りをいつでもお待ちしてます!!

@terapotan
Copy link
Contributor Author

@Hiroshiba @sevenc-nanashi
お久しぶりです。
前の返信から随分経ってしまいました……が、指摘内容を修正に含めました。具体的には、

  1. 最大化ボタンを押しても、全画面表示が解除されるように修正
  2. その他変数名など、指摘に従って修正

の項目について修正を行いました。ご確認いただけますと幸いです。

image
なお、上記に全画面表示と最大化機能に関する状態遷移図と状態遷移表を添付しておきます。
Fullが全画面表示、Maxが最大化表示を表しています。この動作は、FireFoxの動作に倣ったものになります。
この辺り考え始めると意外と複雑で……理解の助けになれば幸いです。

@voicevox-preview-pages
Copy link

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

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

@Hiroshiba
Copy link
Member

@terapotan 遷移図ありがとうございます!!

なるほどです、これフルスクリーン中に最大化ボタンを押すとフルスクリーンがキャンセルされる形になってるんですね!
良さそう!!

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 です!!
ちょっと細かいところコメントしてみました!せっかくなのでご意見伺えると 🙏

細かいところが主なので、特にご意見ない&コード変更が大変であればこちらで変更しちゃいたいと思います。
macは時間ある時にこっちで確かめてみます!

  • macで動くか確かめる

src/backend/electron/main.ts Outdated Show resolved Hide resolved
src/components/Menu/MenuBar/MenuBar.vue Show resolved Hide resolved
src/type/preload.ts Outdated Show resolved Hide resolved
@terapotan
Copy link
Contributor Author

@Hiroshiba
迅速なレビューありがとうございます!
指摘された事項について、特に意見ありませんので、こちら側で修正反映しておきました!

Macでの動作確認については、すみませんがよろしくお願いします。

@Hiroshiba
Copy link
Member

LGTM!!

mac上でのチェックですが、多分大丈夫だと思うので、マージ後の自動ビルドの成果物で試してみようと思います!
ビルドに問題がなければ多分↓にリリースされるはず!

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

macで確かめてみて、ちゃんとフルスクリーンできることを確認しました! 🎉

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.

フルスクリーンを可能にする
3 participants