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

refactor: MenuBarの定義を分散 #2596

Merged
merged 40 commits into from
Mar 27, 2025

Conversation

sevenc-nanashi
Copy link
Member

@sevenc-nanashi sevenc-nanashi commented Mar 6, 2025

内容

MenuBarの定義を分散させます。
Discordはこのへん:https://canary.discord.com/channels/879570910208733277/893889888208977960/1342134282281287750

関連 Issue

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

(なし)

その他

(なし)

@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner March 6, 2025 09:00
@sevenc-nanashi sevenc-nanashi requested review from Hiroshiba and removed request for a team March 6, 2025 09:00
@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Mar 6, 2025

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

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

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.

レビューしてみました!!!
これ難しいですね!!!!!!!

最終的にseparatorごとに分けてMenuItem[]を管理することになると思うので、それを見越してデータ構造を提案してみました!!

group: ["electron"],
regex: "^electron(\\/|$)",
Copy link
Member

Choose a reason for hiding this comment

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

(判断メモコメントです)

該当箇所でno-restricted-importsをignoreする方が目的に合ってる・・・・・かも・・・?
と思ったけど、メインプロセス以外から使われるのはelectronだけだし、これで良さそう!

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です!!!

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.

あと少し感!!!

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reorganizes the MenuBar definitions by distributing them across multiple modules and updating related type definitions and tests. Key changes include:

  • Introducing new composable hooks (e.g., useElectronMenuBarData) and updating MenuBar concatenation logic.
  • Adjusting type definitions and renaming properties (e.g., replacing “disablreloadingLocked” with “disableWhileReloadingLock”).
  • Updating test cases and ESLint/Vite configurations to reflect these refactorings.

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/backend/electron/renderer/menuBarData.ts Introduces electron-specific menu bar data via a new composable.
tests/unit/lib/arrayHelper.ts Adds tests for array helper functions with a minor mismatch in test description naming.
src/components/Menu/MenuBar/menuBarData.ts Consolidates MenuBar data concatenation with a workaround for type mismatches.
tests/unit/domain/menuBarData.spec.ts Provides tests for the new concatMenuBarData behavior.
src/backend/electron/ipc.ts Streamlines IPC types by removing obsolete definitions.
src/helpers/arrayHelper.ts Exports helper functions with no functional changes.
src/components/Menu/MenuBar/useCommonMenuBarData.ts Defines common menu bar data and registers hotkeys; no functional changes aside from refactor.
src/helpers/typedEntries.ts Adds a helper function (mapObjectValues) with inline documentation.
src/components/Talk/menuBarData.ts Adopts the new MenuBar content type and refactors accordingly.
src/components/Menu/type.ts Renames property “disablreloadingLocked” to “disableWhileReloadingLock”.
eslint.config.js Updates the electron import rule to a regex-based approach.
vite.config.ts Updates the preload input file path to use the renderer preload file.
src/components/Sing/menuBarData.ts Refactors menu data for the Sing module to align with the new MenuBar content structures.
src/components/App.vue Integrates new menu bar data sources and updates component prop names accordingly.
src/components/Menu/MenuButton.vue Corrects property name usage for reloading lock checks.
Comments suppressed due to low confidence (3)

tests/unit/lib/arrayHelper.ts:21

  • The test suite description 'removeNonValue' does not match the function name 'removeNullableAndBoolean'. Consider updating the description for clarity.
describe("removeNonValue", () => {

src/components/Menu/MenuBar/menuBarData.ts:62

  • Avoid using '@ts-expect-error' by addressing the underlying type mismatch; consider refining the type definitions for 'itemsArray' and 'itemValue' to remove the need for suppression.
// @ts-expect-error 型が合わないので無視する。

vite.config.ts:129

  • Ensure that changing the preload input file path to 'renderer/preload.ts' aligns with Electron's preload requirements, as the removed comment suggests potential implications.
input: "./src/backend/electron/renderer/preload.ts",

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なのですが、あとでもう1回だけファイルチェックさせていただこうかなと・・・!!!

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!!!!!!!!!!

お疲れ様でした!!かなり良い感じにファクタリングで来たと思います!!!!!
良いリファクタリング筋トレ(?)になりました・・・!!

TODOコメントいくつかあるのでissue化しておくとメンテナっぽいかもです!

<template>
  <ErrorBoundary>
    <TooltipProvider disableHoverableContent :delayDuration="500">
      <!-- TODO: subMenuDataのpropsを分けず、1つにまとめて渡す -->
      <MenuBar
        v-if="openedEditor != undefined"
        :fileSubMenuData="subMenuData.file"
        :editSubMenuData="subMenuData.edit"
        :viewSubMenuData="subMenuData.view"
        :engineSubMenuData="subMenuData.engine"
        :settingSubMenuData="subMenuData.setting"
const store = useStore();
// TODO: useMenuBarData系の関数をcomposableじゃなくする
const commonMenuBarData = useCommonMenuBarData(store);
const talkMenuBarData = useTalkMenuBarData(store);
const singMenuBarData = useSingMenuBarData(store);
const electronMenuBarData = useElectronMenuBarData(store);

↓のissueにぶら下げておくとコミットしやすいかも?

@Hiroshiba Hiroshiba changed the title chore: MenuBarの定義を分散 refactor: MenuBarの定義を分散 Mar 23, 2025
@Hiroshiba
Copy link
Member

@sevenc-nanashi これ早めにマージ目指せると良いかもです・・・!

#2596 (review) にコメント書いています)

@sevenc-nanashi
Copy link
Member Author

#2631
#2632
を建てたのでマージします。

@sevenc-nanashi sevenc-nanashi added this pull request to the merge queue Mar 27, 2025
Merged via the queue into VOICEVOX:main with commit 7007dcf Mar 27, 2025
11 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