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

複数選択:選択だけ実装 #1470

Merged
merged 59 commits into from
Sep 1, 2023

Conversation

sevenc-nanashi
Copy link
Member

@sevenc-nanashi sevenc-nanashi commented Aug 4, 2023

内容

複数選択の選択部分だけ実装します。

関連 Issue

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

_VOICEVOX.-.Ver.999.999.999.Mozilla.Firefox.2023-08-13.07-09-26.mp4

その他

(なし)

@sevenc-nanashi sevenc-nanashi marked this pull request as ready for review August 5, 2023 02:41
@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner August 5, 2023 02:41
@sevenc-nanashi sevenc-nanashi requested review from Hiroshiba and removed request for a team August 5, 2023 02:41
@sevenc-nanashi sevenc-nanashi marked this pull request as draft August 5, 2023 03:07
@sevenc-nanashi
Copy link
Member Author

image

_VOICEVOX.-.Ver.999.999.999.Mozilla.Firefox.2023-08-05.12-19-04.mp4

選択は出来るようになりました。

@sevenc-nanashi sevenc-nanashi marked this pull request as ready for review August 5, 2023 03:22
@sevenc-nanashi
Copy link
Member Author

sevenc-nanashi commented Aug 12, 2023

Ctrl/Shiftを押してるときは当たり判定用のdivを出現させるようにしました。

@sevenc-nanashi
Copy link
Member Author

これ、実験的機能に隔離した方が良いような気がしました(e2eどうやるのって話になりますが)

@Hiroshiba
Copy link
Member

実験的機能

あ、賛成です!!
今は選択できるだけなのであれば、ON/OFF切り替えできるのは開発版だけにしておくと安心かもしれない。

e2eどうやるのって話になりますが

これって実験的機能をonにしたe2eテストどうやるのって意味でしょうか 👀
であればe2eテストの最初で設定の実験的機能をonにすれば良さそう感。

@sevenc-nanashi
Copy link
Member Author

反映しました。

Comment on lines 565 to 567
&[data-is-multi-select-enabled="true"]:is(.selected, .active) {
background-color: rgba(colors.$primary-light-rgb, 0.1);
}
Copy link
Member

@Hiroshiba Hiroshiba Aug 15, 2023

Choose a reason for hiding this comment

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

ライトモードだと緑と白の相性が微妙に良くなくて視認性がちょっと悪めかも・・・?
アクセント区の選択色を流用するのはどうでしょう? 👀
https://github.com/Hiroshiba/voicevox/blob/6e3babad5dfc68a3ec1de59c7c864a960e8a30e8/docs/%E8%89%B2%E3%81%AB%E3%81%A4%E3%81%84%E3%81%A6.md#L20-L22

Copy link
Member Author

@sevenc-nanashi sevenc-nanashi Aug 15, 2023

Choose a reason for hiding this comment

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

そうしました。緑と青の境目が地味に気持ち悪かった(ハレーション?)ので薄めました。
image
image

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.

良い実装なのではと感じました!!
所々不要になったコードが残っていそうだったので一旦それをコメントしてみました。

Comment on lines 94 to 95
// FIXME: Macでは動かないので、Macでは落ちるテストとしてマークする。
test.fail();
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 Author

Choose a reason for hiding this comment

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

わからないですね…CmdとCtrlで色々違うのかも知れません。Mac実機を持ってないのでどうしようもないです

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 Author

Choose a reason for hiding this comment

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

うーん。駄目そう。

@sevenc-nanashi
Copy link
Member Author

レビューを反映しました。

Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
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
Copy link
Member Author

レビューを反映しました。

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.

問題ないと思うのでマージしたいと思います! お疲れ様でした!!

どんどん機能実装していきたいですね・・・!
実装の課題として、パラメータ欄の表示をどうするか、詳細設定欄の表示をどうするか、複数選択時の変更の実装をどうするかなどがありそう。
もし続きを実装する場合、いきなりプルリクでも良いのですが、先にissueで「こういう感じでの実装を考えてます」から始めると、設計を考える訓練になるかもとか思いました。
(もちろんいきなりプルリクでもOKです!)


@thiramisu
以前も共有したかもですが、このあたり(AudioCellのfocus辺り)の実装は以前・これからの実装と関連があるかもなので共有です・・・!
AudioCellにfocusがあたったときはactiveKeyのみがsetされる(Inputにfocusしない)という経路ができた感じかなと。

@sevenc-nanashi
Copy link
Member Author

実装の課題として、パラメータ欄の表示をどうするか、詳細設定欄の表示をどうするか、複数選択時の変更の実装をどうするかなどがありそう。

割と決めてました: #457 (comment)

@Hiroshiba
Copy link
Member

Hiroshiba commented Aug 26, 2023

実装の課題として、パラメータ欄の表示をどうするか、詳細設定欄の表示をどうするか、複数選択時の変更の実装をどうするかなどがありそう。

割と決めてました: #457 (comment)

確かに方針に関してはそんな感じで良さそうなんですが、実装まで見ると結構自明じゃないところはありそうに思います

  • パラメーター変更時に代入し直すのはどうするのか
  • パラメーター欄で「???」と表示する実装はどうするのか(数値限定になってたり)
  • パラメーターが「???」の時のスライダーの表示はどうするのか
  • プリセットが違っていた場合に「???」と表示する実装はどうするのか
  • モーフィングは違っていた場合に「???」と表示する実装はどうするのか

ちょっと実際の実装を見ずに言っているので、まだ他にもある気がします

@sevenc-nanashi
Copy link
Member Author

Google Slidesを参考に:

パラメーター欄で「???」と表示する実装はどうするのか(数値限定になってたり)

image
空欄表示もけっこうありますね。

パラメーターが「???」の時のスライダーの表示はどうするのか

image
異なる場合はデフォルトを出しているようです。

プリセットが違っていた場合に「???」と表示する実装はどうするのか

image
異なっていたら空欄でした。

モーフィングは違っていた場合に「???」と表示する実装はどうするのか

image
1つでもオフになっていたらオフになっていました。

全て、一つ設定したら全てが同じ値になっていました。

@Hiroshiba
Copy link
Member

なるほどぉ。
Google Slideの複数選択時の値が異なるときの挙動、デフォルト値表示などちょくちょく不自然な気はしますが、これくらい思い切った実装でもそこまで不都合ない感じかもですね。

コメントした意図としては、課題になりそうな詳細箇所を探しておいて、あとあと実装で行き詰まるのを少なくするのはどうかという感じでした。
(与えられた問題を解くのではなく、問題になりそうな点を探す感じ・・・?)

@Hiroshiba
Copy link
Member

あ、マージしていませんでした 🙇

複数変更機能はニーズが高いと思います、もしよかったらぜひ・・・!!!

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