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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

terapotan
Copy link

@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
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.

細かいですが、右上の全画面のボタンも連動させた方がいいと思います。
具体的には:フルスクリーンでも最大化解除のアイコンにして、それを押してもフルスクリーンが解除されてもいいと思います。

subMenu: [...props.viewSubMenuData],
subMenu: [
...props.viewSubMenuData,
{ type: "separator" },
Copy link
Member

Choose a reason for hiding this comment

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

(細かいですが)トークエディタだと一番上にseparatorが来そう?

Copy link
Member

Choose a reason for hiding this comment

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

ほんとですね!! ちょっと見た目変になりそう。
残しといていただいたら後でちょっとこちらで調整させていただきます! 🙏

Copy link
Author

Choose a reason for hiding this comment

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

コメントありがとうございます!
@Hiroshiba
個人的な感想として、フルスクリーンモード切替ボタンと他のボタンをセパレーターで区切る意味はそんなにないかなと思います。このPRにおいては、セパレーターを消す方向で修正進めてもいいでしょうか?(一旦残して後から修正入れていただく方向でも大丈夫です!)

Copy link
Member

Choose a reason for hiding this comment

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

大丈夫だと思います。

Copy link
Member

Choose a reason for hiding this comment

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

たしかに、一旦なしがとりあえずで一番良さそう!ご提案ありがとうございます!

@@ -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
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
Author

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

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

@Hiroshiba
Copy link
Member

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

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

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