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

Add: シーケンサにおいてツール選択可能にする #2367

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

romot-co
Copy link
Contributor

内容

以下の機能を追加します

  • ノート編集において、編集優先ツールと選択優先ツールを切り替えられるようにする
  • ピッチ編集において、ドローツールと削除ツールを切り替えられるようにする

関連 Issue

ref #2039
close #2039

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

スクリーンショット 2024-11-17 12 17 15
test-tools-scr.mp4

その他

ツールパレットの位置やUI構成については考慮必要かもしれないですが、
まずは機能追加を行いボリューム編集を取り込み後に適宜修正できればと考えています

Material Iconsに適切なものがなかったため
Material Symbolsを取り入れています(別Issueの方がよさそうでしたらおしらせください)

※ 再作成します

@romot-co romot-co requested a review from a team as a code owner November 19, 2024 15:19
@romot-co romot-co requested review from Hiroshiba and removed request for a team November 19, 2024 15:19
@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Nov 19, 2024

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

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

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 11 changed files in this pull request and generated no suggestions.

Files not reviewed (5)
  • src/styles/_index.scss: Language not supported
  • src/styles/fonts.scss: Language not supported
  • src/components/Sing/ScoreSequencer.vue: Evaluated as low risk
  • src/components/Sing/SequencerNote.vue: Evaluated as low risk
  • src/components/Sing/SequencerToolPalette.vue: Evaluated as low risk
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/SequencerToolPalette.vue Outdated Show resolved Hide resolved
src/composables/useEditMode.ts Outdated Show resolved Hide resolved
src/store/type.ts Outdated Show resolved Hide resolved
Comment on lines 477 to 524
// カーソル状態の管理
const { setCursorState, cursorState, cursorClass } = useCursorState();

// TODO: カーソル状態を決定するロジックをuseCursorStateに移譲もしくはステートパターン実装
const resolveCursorState = () => {
// プレビュー中は現在の状態を維持
if (nowPreviewing.value) {
return cursorState.value;
}

if (editTarget.value === "PITCH") {
// ピッチ編集モード時のカーソル状態
return ctrlKey.value || selectedPitchTool.value === "ERASE"
? CursorState.ERASE
: CursorState.DRAW;
}
if (editTarget.value === "NOTE") {
// ノート編集モード時のカーソル状態
if (shiftKey.value) {
// 範囲選択
setCursorState(CursorState.CROSSHAIR);
} else {
setCursorState(CursorState.UNSET);
return CursorState.CROSSHAIR;
}
return selectedNoteTool.value === "EDIT_FIRST"
? CursorState.DRAW
: CursorState.UNSET;
}
});

return CursorState.UNSET;
};

// カーソル状態の更新を一元管理
const updateCursorState = () => {
const newState = resolveCursorState();
setCursorState(newState);
};

// 関連する状態が変更されたときにカーソル状態を更新
watch(
[
ctrlKey,
shiftKey,
nowPreviewing,
editTarget,
selectedNoteTool,
selectedPitchTool,
],
updateCursorState,
{ immediate: true },
);
Copy link
Member

Choose a reason for hiding this comment

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

(ただのコメントです)

watch含めてuseCursorStateに持っていける気がしますね!
watchが見てる変数を全部useCursorStateの引数に渡しちゃう感じで。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちらもたぶんステートマシンにも関係してきそう?なので
ScoreSequencerに残す形で修正しました…!

Comment on lines +277 to +278
.material-symbols-outlined {
font-family: "Material Symbols Outlined";
Copy link
Member

Choose a reason for hiding this comment

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

Material Symbols

ちょっと軽い質問なのですが、Material Iconsを使うかこっちを使うかの線引きとかってどう考えれば良いでしょうか? 👀
全体的な調和が取れていればどっちを使っても良いとしたり、ここではこうすべきみたいな方針が見えていればお聞きしたく・・・!!

導入しても良さそうに感じるのですが、選択肢が2倍になったり、他のアイコンも導入したいとなった時の方針をちょこっと考えておきたいな~と。

Copy link
Contributor Author

@romot-co romot-co Nov 22, 2024

Choose a reason for hiding this comment

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

@Hiroshiba
こちらざっくり

  1. 当面はMaterial Iconsに無いものがあればMaterial Symbolsを使う
  2. 新しく追加するものは全部Material Symbolsにする
  3. 全部Material Symbolsにしてしまう

の3パターンかと思いまして
個人的には採用の可否・コンポーネント含め利用法固まったら2.かな?と思っています


Material IconsとMaterial Symbolsの差

同一名であれば存在し見た目は同系列

Material SymbolsはMaterial Iconsの後継・スーパーセットなので
Material Iconsにあるものは置き換えられ印象に差は少なく移行は楽かと思います

よりモダンっぽい印象です

いちおう結構形まで変わっているものはあり:

UNDO
https://fonts.google.com/icons?selected=Material+Symbols+Outlined:undo:FILL@0;wght@400;GRAD@0;opsz@24&icon.query=undo&icon.size=24&icon.color=%235f6368

https://fonts.google.com/icons?selected=Material+Icons+Outlined:undo:&icon.query=undo&icon.size=24&icon.color=%235f6368&icon.set=Material+Icons

増えてるものはそこそこある:

今回追加のカーソルアイコン:
https://fonts.google.com/icons?selected=Material+Symbols+Outlined:arrow_selector_tool:FILL@0;wght@400;GRAD@0;opsz@24&icon.query=tool&icon.size=24&icon.color=%235f6368&icon.style=Outlined

Variable Fontで調整ができる

VOICEVOXであればこれみたいなスタイルをちょっと調整できる
※ ただし既存と印象が揃わなくなるため全部入れ替え時に調整がいいかも

  • ウェイトをCSSで指定できる
  • 細かい太さを調整できる
  • サイズに応じて線の太さを自動調整してくれる

そもそもMaterial Icon/Symbol系を使わず他のアイコンセットにするのもありと思っていますが
Material系+他のアイコンセットの混在は避けた方がいいかもです

Choose a reason for hiding this comment

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

Copilot reviewed 7 out of 11 changed files in this pull request and generated no suggestions.

Files not reviewed (4)
  • src/styles/_index.scss: Language not supported
  • src/styles/fonts.scss: Language not supported
  • src/components/Sing/SequencerNote.vue: Evaluated as low risk
  • src/components/Sing/SequencerToolPalette.vue: Evaluated as low risk
Comments skipped due to low confidence (1)

src/components/Sing/ToolBar/EditTargetSwicher.vue:35

  • The comment 'transitionHide' seems to be a typo. It should be 'transitionSide'.
transitionHide=""

Choose a reason for hiding this comment

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

Copilot reviewed 7 out of 11 changed files in this pull request and generated no suggestions.

Files not reviewed (4)
  • src/styles/_index.scss: Language not supported
  • src/styles/fonts.scss: Language not supported
  • src/components/Sing/SequencerNote.vue: Evaluated as low risk
  • src/components/Sing/SequencerToolPalette.vue: Evaluated as low risk

Choose a reason for hiding this comment

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

Copilot reviewed 7 out of 11 changed files in this pull request and generated no suggestions.

Files not reviewed (4)
  • src/styles/_index.scss: Language not supported
  • src/styles/fonts.scss: Language not supported
  • src/components/Sing/SequencerNote.vue: Evaluated as low risk
  • src/components/Sing/SequencerToolPalette.vue: Evaluated as low risk
Comments skipped due to low confidence (1)

src/components/Sing/ToolBar/EditTargetSwicher.vue:35

  • Attribute name 'transitionSide' should be 'transitionHide'.
transitionHide=""
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants