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

前回開いていたエディタ(トーク or ソング)画面を起動時に表示 #2355

Merged
merged 9 commits into from
Nov 19, 2024

Conversation

X-20A
Copy link
Contributor

@X-20A X-20A commented Nov 15, 2024

内容

とりあえず動くはずですが、理解して書いたというよりもエラーメッセージの誘導に従っただけな感があるので、設計の方針に沿うかはなんとも分からないです

関連 Issue

#2010

@X-20A X-20A requested a review from a team as a code owner November 15, 2024 23:35
@X-20A X-20A requested review from Hiroshiba and removed request for a team November 15, 2024 23:35
@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Nov 15, 2024

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

更新時点でのコミットハッシュ:6f91632

src/components/App.vue Outdated Show resolved Hide resolved
src/store/ui.ts Outdated
Comment on lines 347 to 349
mutations.SET_OPENED_EDITOR({
editor: await window.backend.setSetting("editorType", editorType),
});
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
mutations.SET_OPENED_EDITOR({
editor: await window.backend.setSetting("editorType", editorType),
});
await window.backend.setSetting("editorType", editorType)
mutations.SET_OPENED_EDITOR({
editor: editorType,
});

普通に分けた方が良さそう。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

私もそちらが分かりやすそうに思えます。ただ、既存のSET_INHERIT_AUDIOINFOやSET_ACTIVE_POINT_SCROLL_MODEでは引数内で記述されているので統一感を考慮してSET_EDITOR_TYPEもそれに倣いました。これらの関数がどのような意図や都合で書かれたか分からないため、判断に迷っています。

Copy link
Member

Choose a reason for hiding this comment

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

確かにこれ混乱しますね!!
僕も正確なところがちょっとわかんないです。。。

最初は設定ごとにSETのためのmutation/actionがあったのですが、後々に統合されたSET_ROOT_MISC_SETTINGができました。
なので大多数はSET_ROOT_MISC_SETTINGを使ってますが、どうやらまだ個別にSETしているものが残っていたっぽいです・・・!

今回の場合は多分SET_EDITOR_TYPEは用意せず、SET_ROOT_MISC_SETTINGを使う形が良さそうに感じました!

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ありがとうございます!!!
実装とコメントを見させていただいて、確かにちょっとややこしくなっていることがわかりました 🙇
ということで、整理した後提案を書いてみました!


既存で「どれが開かれているか」を格納しているopenedEditorと、それとは別にeditorTypeが状態(State)に含まれていますね!
実際にUIの表示に使われているのはopenedEditorで、後者は設定の同期のために使われていそう。
つまり設定の同期が終わった後は同じものを示す変数が2つあり、このままだと何が取りに依存するかわからなくなって難しいことになるかも。

ちょっと提案なのですが設定+状態のeditorTypeを追加せず、逆に今までのopenedEditorを設定に追加するのはどうでしょうか? 👀
多分方針はこんな感じ・・・?

  • settingにopenedEditorを追加
    • 型は"talk" | "song"だけでundefinedは無し
    • デフォルト値を"talk"にする
  • 元からあったuiState.openedEditorをなくし、serringState.openedEditorに移動
  • あとは微調整して完成?

ちょっとややこしいと思うので、多分こんな感じというのをコメントしていきます!!
コメントの内容以外に↓もすれば完成・・・・なはず・・・!!

  • SET_OPENED_EDITORを消す
    • SET_OPENED_EDITORでファイル全体検索すれば・・・!
  • 代わりにSET_ROOT_MISC_SETTING("openedEditor")を使う

不明だったら何でもコメントください 🙇

Comment on lines 80 to 83
await store.dispatch("SET_ROOT_MISC_SETTING", {
key: "editorType",
value: openedEditor,
});
Copy link
Member

Choose a reason for hiding this comment

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

ここはなくなる

Comment on lines 117 to 120
// どちらのエディタを開くか設定
await store.actions.SET_OPENED_EDITOR({ editor: "talk" });
await store.actions.SET_OPENED_EDITOR({
editor: store.state.editorType,
});
Copy link
Member

Choose a reason for hiding this comment

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

ここは以前のコードもなくなる
SET_OPENED_EDITORを呼ばない)

@@ -71,6 +71,7 @@ export const settingStoreState: SettingStoreState = {
},
showSingCharacterPortrait: true,
playheadPositionDisplayFormat: "MINUTES_SECONDS",
editorType: "talk",
Copy link
Member

Choose a reason for hiding this comment

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

openedEditorに

@@ -148,6 +149,7 @@ export const settingStore = createPartialStore<SettingStoreTypes>({
"undoableTrackOperations",
"showSingCharacterPortrait",
"playheadPositionDisplayFormat",
"editorType",
Copy link
Member

Choose a reason for hiding this comment

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

ここもopenedEditor

Comment on lines 1924 to 1925
editorType: EditorType;
openedEditor: EditorType | undefined; // undefinedのときはどのエディタを開くか定まっていない
Copy link
Member

Choose a reason for hiding this comment

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

ここはuiStateなので、editorTypeもopenedEditorも両方消す

src/store/ui.ts Outdated
Comment on lines 66 to 67
editorType: "talk",
openedEditor: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

ここも消えるはず

src/store/ui.ts Outdated
Comment on lines 264 to 267
mutations.SET_OPENED_EDITOR({
editor: await window.backend.getSetting("editorType"),
});

Copy link
Member

Choose a reason for hiding this comment

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

HYDRATE_SETTING_STORE内のmutations.SET_ROOT_MISC_SETTINGがあるのでこのコードはなくても良いはず!

(さらに言うと上のmutations.SET_ACTIVE_POINT_SCROLL_MODEとかもいらなそうだけど・・・まあ一旦放置で・・・。
あ、もし気が向いたら別でプルリクエストとかいただけるとめちゃくちゃ嬉しいです!!)

src/store/ui.ts Outdated
Comment on lines 343 to 347
SET_EDITOR_TYPE: {
mutation(state, { editorType }: { editorType: EditorType }) {
state.editorType = editorType;
},
async action({ mutations }, { editorType }: { editorType: EditorType }) {
Copy link
Member

Choose a reason for hiding this comment

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

ここもなくせそう!

@@ -594,6 +594,7 @@ export const rootMiscSettingSchema = z.object({
playheadPositionDisplayFormat: z
.enum(["MINUTES_SECONDS", "MEASURES_BEATS"])
.default("MINUTES_SECONDS"), // 再生ヘッド位置の表示モード
editorType: z.enum(["talk", "song"]).default("talk"),
Copy link
Member

Choose a reason for hiding this comment

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

ここをopenedEditorに変える感じで・・・!
(これが今回の提案の肝)

@@ -603,6 +604,7 @@ export const configSchema = z
activePointScrollMode: z
.enum(["CONTINUOUSLY", "PAGE", "OFF"])
.default("OFF"),
editorType: z.enum(["talk", "song"]).default("talk"),
Copy link
Member

Choose a reason for hiding this comment

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

rootMiscSettingSchema側にあるのでこちらいらないはず!

@Hiroshiba
Copy link
Member

テストが落ちてそうでした!!

調べてみた感じかなり難しかったので、こちらでPR作ってみました!
原因はショートカットキーがなぜか登録されないためだったので、ちゃんと実行するようにしてみました 🙏

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.

変更ありがとうございます!!
もうほとんど感染してる気がします!!

これソングで開いたあと保存して再度開いた時、一瞬だけトーク画面が映っちゃいますね!!

Vuex.stateの初期値はundefinedにし、保存されている値の初期値talkにして、undefinedのときは(以前と同様に)何も映さないようにすれば解決しそう・・・?
ということでまたになってしまうのですがプルリクエスト作ってみました 🙇

何かおかし挙動とかあれば・・・・・ 🙇

@X-20A
Copy link
Contributor Author

X-20A commented Nov 18, 2024

ありがとうございます。
npm run electron:serveしてみると作動はしますがいくつかエラーが出るようです。
更新との関係がなかなか読み難い感じです。
キャプチャ_2024_11_19_06_49_45_81

@sevenc-nanashi
Copy link
Member

多分ローカルでnpm iし直したら治ると思います

@X-20A
Copy link
Contributor Author

X-20A commented Nov 18, 2024

ありがとうございます。
だいぶ消えたのでまた見直してみます

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.

大丈夫だと思います。

@Hiroshiba
Copy link
Member

LGTM!!!

実装ありがとうございました!!

またお誘いになってしまうのですが、もしよかったらまた機能追加に挑戦してみませんか・・・!!
できそうなissueを探してみました!!

もしよければぜひぜひ!!!
(ちょっと難度が上がってると思うので、不明なとこがあれば何でも聞いていただければ!!)

@Hiroshiba Hiroshiba merged commit fc0784e into VOICEVOX:main Nov 19, 2024
10 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.

3 participants