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

ソング:歌詞の一括入力を追加 #1952

Merged
merged 33 commits into from
Apr 17, 2024

Conversation

sevenc-nanashi
Copy link
Member

@sevenc-nanashi sevenc-nanashi commented Mar 23, 2024

内容

タイトル通り。

関連 Issue

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

multi-mora.mp4

その他

モーラ分割とかの都合上長音対応も入ってます。

@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner March 23, 2024 09:23
@sevenc-nanashi sevenc-nanashi requested review from y-chan and removed request for a team March 23, 2024 09:23
src/sing/domain.ts Outdated Show resolved Hide resolved
src/components/Sing/SequencerNote.vue Outdated Show resolved Hide resolved
@Hiroshiba Hiroshiba requested review from Hiroshiba and removed request for y-chan March 27, 2024 00:18
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.

概ね良さそうでした!!! きれいな実装だと思います!

このPRの内容は実は以前課題があるよねと議論していたものだったので、そこは別で議論できればと思います 🙏
#1815 (comment)
(たぶんこの機能とUXのままマージできる・・・かも?)

src/components/Sing/SequencerNote.vue Outdated Show resolved Hide resolved
src/components/Sing/SequencerNote.vue Outdated Show resolved Hide resolved
src/components/Sing/ScoreSequencer.vue Show resolved Hide resolved
src/components/Sing/SequencerNote.vue Outdated Show resolved Hide resolved
src/components/Sing/SequencerNote.vue Outdated Show resolved Hide resolved
src/sing/domain.ts Outdated Show resolved Hide resolved
src/sing/domain.ts Outdated Show resolved Hide resolved
src/sing/domain.ts Outdated Show resolved Hide resolved
src/sing/domain.ts Outdated Show resolved Hide resolved
tests/unit/lib/splitMorasAndNonMoras.spec.ts Outdated Show resolved Hide resolved
sevenc-nanashi and others added 10 commits March 27, 2024 15:42
Co-Authored-By: Hiroshiba <Hiroshiba@users.noreply.github.com>
Co-Authored-By: Hiroshiba <Hiroshiba@users.noreply.github.com>
Co-Authored-By: Hiroshiba <Hiroshiba@users.noreply.github.com>
Co-Authored-By: Hiroshiba <Hiroshiba@users.noreply.github.com>
Co-Authored-By: Hiroshiba <Hiroshiba@users.noreply.github.com>
@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.

もうほぼ lgtm です!!
シンプルでいいコードだと思いました!

ちょっといくつかめちゃくちゃ細かい提案をしています。
もしよかったら!

src/components/Sing/SequencerNote.vue Outdated Show resolved Hide resolved
src/components/Sing/SequencerNote.vue Outdated Show resolved Hide resolved
src/components/Sing/ScoreSequencer.vue Outdated Show resolved Hide resolved
src/composables/useLyricInput.ts Show resolved Hide resolved
src/composables/useLyricInput.ts Outdated Show resolved Hide resolved
src/composables/useLyricInput.ts Outdated Show resolved Hide resolved
Co-Authored-By: Hiroshiba <Hiroshiba@users.noreply.github.com>
@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.

LGTM!!

Comment on lines +402 to +408
const onNoteLyricInput = (text: string, note: Note) => {
splitAndUpdatePreview(text, note);
};

const onNoteLyricBlur = () => {
commitPreviewLyrics();
};
Copy link
Member

Choose a reason for hiding this comment

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

ちなみにreactではemit側をon〜、引数指定側をhandle〜にしてたりします。
https://ja.react.dev/learn/responding-to-events#adding-event-handlers
ややこしくなったらそういうルールにしても良さそう。

src/composables/useLyricInput.ts Outdated Show resolved Hide resolved
src/composables/useLyricInput.ts Outdated Show resolved Hide resolved
@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.

すみません、approveした後なのですが2つ課題を見つけました 🙇

1つ目はいっぱい入力したときにinputの後ろでノートの表示が重なって見える点です。
inputじゃない方はpreviewか、あるいは何も表示しないのが良さそう?
解決できそうなら解決しちゃえると嬉しいです。
image

src/sing/domain.ts Outdated Show resolved Hide resolved
@sevenc-nanashi
Copy link
Member Author

直ったと思います。

@sevenc-nanashi sevenc-nanashi requested a review from Hiroshiba April 5, 2024 14:27
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.

ちょっと挙動に問題がないか自信がないので、リファクタリングをお願いできると・・・・・ 🙇

src/components/Sing/SequencerNote.vue Show resolved Hide resolved
src/sing/domain.ts Outdated Show resolved Hide resolved
@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.

LGTM!!

ちょっとこちらでいくつか変更させていただきます!

src/components/Sing/SequencerNote.vue Outdated Show resolved Hide resolved
src/sing/domain.ts Outdated Show resolved Hide resolved
if (lastMatchEnd < text.length) {
moraAndNonMoras.push(text.substring(lastMatchEnd));
}
if (moraAndNonMoras.length > maxLength) {
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
if (moraAndNonMoras.length > maxLength) {
// 指定した最大要素数より多い場合は配列を削る
if (moraAndNonMoras.length > maxLength) {

Comment on lines 452 to 456
export const convertKanaToHira = (text: string): string => {
return text.replace(/[\u30A1-\u30F4]/g, (s) => {
return String.fromCharCode(s.charCodeAt(0) - 0x60);
});
};
Copy link
Member

Choose a reason for hiding this comment

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

これ不要になりましたね

@Hiroshiba Hiroshiba merged commit 0be6de9 into VOICEVOX:main Apr 17, 2024
9 checks passed
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