UPDATE: コンテキストメニューの内部処理の変更 #1364

thiramisu · 2023-07-01T20:06:24Z

内容

今後の機能追加などをしやすいように、ユーザー視点の見た目・機能が変わらないように内部実装だけを変更しました。
また、AudioCell(テキスト欄)上での操作しか想定されていなかったため、他の場所での操作を追加したくなった時のために、拡張性を高めました。

その他

初PRのため、以下の点をどうするべきか分かりませんでした。実際の実装含め、酷い場合はPRをご却下ください…お手数をおかけします。

コメントをどこに書くべきか分からず、ソースコード中に書いています。
PRを送る前に清書をしてgithub側のコメントに移す形をとるべきでしたでしょうか？
src/type/contextMenu.ts 内に将来実装のために書いた部分がコメントアウトされていない状態で存在します。

今後の機能追加などをしやすいように、ユーザー視点の見た目・機能が変わらないように実装を変更 mainブランチを使っていたので更新箇所を別ブランチへ切り出し。

thiramisu · 2023-07-01T20:10:19Z

src/electron/contextMenu.ts

+      label: "全選択",
+      role: "selectAll",
+    },
+  ] as ContextMenuItemConstructorOptions<TextEditContextMenuAction>[],


ここの as ~ はデフォルト設定を追加する際にtypoなどの判定をさせたくてあえて追加しました。
うまく実装すればこの場所に書かなくても同じことをできそうですが、良いアイディアが思いつきませんでした。

Hiroshiba

PRありがとうございます！！

ちょっと確認なのですが、このあと雰囲気的に将来レンダラープロセス側（src/store内とか*.vue内など）からtemplateをsrc/electron/contextMenu.tsに渡してMenu.buildFromTemplateでカスタマイズ可能にするといった感じでしょうか 👀

この場合、もしかしたらContextMenuClickCallbackでレンダラープロセス側から実行したい関数を渡すことができないかもです。（ipc通信でコールバック関数を渡せないため）
把握されていたら余計なお世話なのですみません 🙇‍♂️

Hiroshiba · 2023-07-02T04:21:47Z

src/background.ts

@@ -735,8 +735,8 @@ ipcMainHandle("SHOW_IMPORT_FILE_DIALOG", (_, { title }) => {
  })?.[0];
 });

-ipcMainHandle("OPEN_TEXT_EDIT_CONTEXT_MENU", () => {
-  textEditContextMenu.popup({ window: win });
+ipcMainHandle("OPEN_CONTEXT_MENU", (_, { type }) => {


typeは予約語なので避けても良いかもとちょっとだけ思いました！
menuTypeとかkindとか。
（ちょっと気になった程度なのでそのままでもOKです！）

Hiroshiba · 2023-07-02T06:39:15Z

src/type/contextMenu.ts

+import { z } from "zod";
+
+export const textEditContextMenuActionSchema = z.enum([
+  "切り取り",
+  "コピー",
+  "貼り付け",
+  "全選択",
+]);
+export type TextEditContextMenuAction = z.infer<
+  typeof textEditContextMenuActionSchema
+>;
+
+// 新たに追加したい場合はここに & で繋げる
+export type ContextMenuAction = TextEditContextMenuAction;
+
+export const ContextMenuTypeShema = z.object({
+  TEXT_EDIT: textEditContextMenuActionSchema,
+});
+const ContextMenuTypes = ContextMenuTypeShema.keyof();
+export type ContextMenuType = z.infer<typeof ContextMenuTypes>;


あ、こんな感じで実行時にjsonからparseしたりする予定がなければzodは使わなくてもOKです！

voicevox/src/store/project.ts

Line 279 in 6ff3a95

const parsedProjectData = projectSchema.parse(projectData);

zod使わなければこんな感じのすっきりした型宣言が書けるはず。まあ、どちらでも！

export type TextEditContextMenuAction = | "切り取り" | "コピー" | "貼り付け" | "全選択"; // 新たに追加したい場合はここに & で繋げる export type ContextMenuAction = TextEditContextMenuAction; export type ContextMenuType = "TEXT_EDIT";

Hiroshiba · 2023-07-02T07:10:56Z

src/electron/contextMenu.ts

+// 返り値の型は Electron.Menu | undefined ではなく Electron.Menu 確定のはずですがどうすればいいか分かりません
+const checkOrBuildContextMenu = (type: ContextMenuType) => {
+  if (menus.has(type)) return menus.get(type);


まあhasがtrueならgetがundefinedにならないのはプログラマ的には明確なので、こうとかでしょうか。

Suggested change

// 返り値の型は Electron.Menu | undefined ではなく Electron.Menu 確定のはずですがどうすればいいか分かりません

const checkOrBuildContextMenu = (type: ContextMenuType) => {

if (menus.has(type)) return menus.get(type);

const checkOrBuildContextMenu = (type: ContextMenuType) => {

if (menus.has(type)) {

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion

return menus.get(type)!;

}

厳密にいうと別スレッドなどでdeleteされる可能性はなくもないので、より正確にはthrowするほうが合ってるかもです。

Suggested change

// 返り値の型は Electron.Menu | undefined ではなく Electron.Menu 確定のはずですがどうすればいいか分かりません

const checkOrBuildContextMenu = (type: ContextMenuType) => {

if (menus.has(type)) return menus.get(type);

const checkOrBuildContextMenu = (type: ContextMenuType) => {

if (menus.has(type)) {

const menu = menus.get(type);

if (menu == undefined) throw new Error("menu is undefined");

return menu;

}

thiramisu · 2023-07-02T08:52:16Z

Discordの方での議論の結果をこちらにもまとめておきます。

そもそもelectronからimportしたくないのは「electronへの依存を抑えたい」のが根底にあるというのが明確になりました。
「electronも含めたe2eテストが大変なので、HTML・Vue周りだけ切り離したい」といった理由のためということです。
このため、無理やり外部に定義しなおすのはその要求を満たしていないため、別の方法を考える必要があります。

ということでひとまずは、コピー・ペースト・切り取り・全選択をピュアに近い実装でやってみてはどうか、という流れになりました。

thiramisu · 2023-07-02T09:13:36Z

レビューありがとうございます。

レンダラープロセス側から実行したい関数を渡すことができない

上記のようにelectron依存から脱却し、.vueファイル化するなどすれば色々やりようはある気がするのですが、もしかしたらまずいかもしれません。検討してみます。

この先も試作してみてはいたのですが、ちょうど謎のエラーで引っかかっていました！
可能性の一つとしては考えていたんですが、多分これですね。

Hiroshiba · 2023-07-02T11:14:50Z

説明が不足していました 🙇‍♂️

無理やり外部に定義しなおすのはその要求を満たしていない

定義し直した場合はelectronの依存から外せているので、一応方針としては大丈夫ではあったりします。（interfaceが同じなだけ）

この先も試作してみてはいたのですが、ちょうど謎のエラーで引っかかっていました！
可能性の一つとしては考えていたんですが、多分これですね。

なるほどです･･･！

このPRの提案ですが、汎用的になったopenContextMenu周りのマージを目指す形はどうでしょうか？
別のPRを作ったほうが良さそうということであればcloseでも問題ありません。進めやすい方で･･･！

thiramisu · 2023-07-02T13:50:10Z

俯瞰的に見た設計について知識不足なところがあり、正直あまり正しく受け取れてないかもです。こちらこそすみません。
結局、今のままだと以下のimportが残りますが、このままで大丈夫なのでしょうか？
https://github.com/thiramisu/voicevox/blob/fbaa8fc4695f8a7e4a4b1d6e4e98ad7a1f0761e7/src/electron/contextMenu.ts#L1

例えば、メニューバーと同じ感じで

今のContextMenu.tsに相当する内容と、コピペ・全選択などの実装をまとめた/components/ContextMenu.vueを新規作成
見た目はQuaserで整える

にすれば、

electronから切り離せる
ContextMenuだけ他のUI定義ファイルから離れたフォルダにある現状のファイル構造が改善される
PCブラウザ版とelectron版で見た目が変わらなくなる
- (UX/UI設計の観点から見た場合にwebアプリとしてなら既定の操作を無視しても大丈夫か自信はないです)
- (あとは先の話になりそうですがスマホ対応となると未知数です)

のではと考えました。動作変更の実装はchangeActiveAudioCell()的な奴の内部からstore.dispatch("CHANGE_CONTEXT_MENU_TARGET", { menuType: "TEXT_EDIT", audioKey: activeAudioKey });とかになるでしょうか。

まあ、ひとまず「"TEXT_EDIT"を引数として切り出す」に留めてcloseなりが良さそうですね！思ったより奥が深い問題でした…

Hiroshiba · 2023-07-02T20:56:56Z

今気づいたのですが、ネガティブな面はない気がしてきました！！
テキスト欄での右クリックに該当する処理はブラウザやスマホに標準搭載されているな、と。

ただ、electronに依存していてもネガティブな面は無いのですが、逆にポジティブな面も消えているのが課題かもです。
そもそもの目的だった「見た目上のテキストだけを編集」が実装できないので･･･。（正確には不可能ではないけどかなり大変）

他にも右クリックに実装したい機能がちらほらあったりして、それらの実装に近づけるというのも利点だと思います！

辞書の単語追加・編集UIの単語・読み入力欄で右クリックメニューを使えるようにする #747

なので、もしよければ/components/ContextMenu.vueに挑戦してみてプルリクエストを頂けると嬉しいです！！
（VuexとVueが入ってくるので更に難しくなると思います、不明な点があればこことか元issueとかで聞いていただければ！）

thiramisu · 2023-07-04T18:09:50Z

[追記]
すみません、確認不足でした！
ほぼバグだったため、直せれば直すことにします！
初めに報告したものは特定状況下でしか起こらない上、根本原因は違いそうでした！
具体的な再現手順は以下の通りになります。

テキスト欄に適当な文字列を入力
そのテキストを範囲選択
テキスト欄からフォーカスを外す
手順2.で選択していた領域でクリックを長押し
クリックを離す

この4.から5.の間、なぜか以前の選択範囲が表示され続けると思います。これがバグの根本原因のようです。
さらにこのバグの発展技として、4. でクリックの代わりに右クリックをしてコンテキストメニューを開くと、
なぜか2.の状態の選択範囲のままコンテキストメニューが開いてしまい、その範囲に対しての操作となってしまう、
というのが、以下に認識を誤って報告をしてしまった動作の原因でした。
[追記ここまで]
[追記２]

QInputのサンプルでは起こってないが、なぜなのかは不明。
以前の選択範囲を復元するのはinput.focus()のデフォルト動作の模様。

[追記２ここまで]
[追記３]

index.htmlの#app外に<input>を置いた場合も発生するので、Electronの仕様の模様。

[追記３ここまで]

~~移植元の右クリック周りの動作をVer 0.14.7で確認していたのですが、怪しい挙動を見つけてしまいました。~~
~~以下は全て、「範囲選択をせずに右クリックしてコンテキストメニューを開く」という操作をした場合の話です。~~
~~あまり見かけない気がする仕様として、開いた時点で自動的にテキストが全選択されます。~~
~~これは~~

~~「[コピー]や[切り取り]をした場合に、テキスト全域が対象になる」ということは明示できるのですが、~~
~~[貼り付け]で「挿入だけする」操作をできません。~~

~~(大多数の方はショートカットキーを使っていそうなので支障などはないのだと思いますが…)~~
~~これはそのまま移植すべきでしょうか？~~
~~自分は1.のメリットより2.のデメリットの方が大きいと思います。~~

Hiroshiba · 2023-07-04T18:49:13Z

ご報告ありがとうございます！！
フォーカスを外したとき・当てたとき・範囲選択をしたときとかでなにかよからぬことが発生していそうですね･･･。

結構レアなケースなのでいったんパスでも良いかもと思いました！！
エゴサしているときに同じような話を見かけた際はご案内したいと思います。ありがとうございます！！

thiramisu · 2023-07-05T20:59:04Z

.vue化、八割がた完成したのですが、1つ、UX的に良くない挙動が生まれてしまいました。
自分はこの挙動はPR前に修正すべきだとは思うのですが、原因に皆目見当がつかず、修正に時間がかかりそうなので、一応修正すべきか否か確認を取りたいです。

挙動の詳細:
コンテキストメニューを開いている間、テキスト選択範囲 (青の反転) などが表示されなくなります。

設計の概要:
既存のMenuItem.vueをコンポーネントとして使用し、<q-menu touch-position context-menu><q-list dense>で囲いました。

調査したことなど:

コンテキストメニューのアクションの実行自体はうまくいきます。
- コンテキストメニューを閉じた時に、開いた時点のテキスト選択範囲が(多分Quasarによって)復元されるためと思われます。
QuasarのContext Menu example (※アンカーがずれるようですが少し下にあります) にcodepenで適当な文字列を付け足した場合は、期待通りテキスト選択範囲が表示されたままコンテキストメニューが開きます。
- [追記: 上記は<div>内にテキストを追加して試したのですが、<input>内の選択範囲は通常の選択範囲とは別の実装で操作する必要があるのを考慮すると、<q-input>内では別の結果になる可能性があります。]
開発者ツールから<input>のcontextmenuイベント以外の全イベントリスナーを削除しても、直りませんでした。
<input>のcontextmenuイベントの (Quasar由来の) リスナーを外したら、期待通り右クリックをしても選択範囲が解除されなくなりました。(が、当然コンテキストメニューも開かなくなります)
views/EditorHome.vueの// ショートカットキーの設定の付近で

  // デフォルトのコンテキストメニューの無効化
  document.addEventListener("contextmenu", (event) => {
    event.preventDefault();
  });

しても、直りませんでした。

「コンテキストメニューを開いてから1秒後に選択範囲を変更する」という実装を試したのですが、少なくとも見た目には反映されないようでした。
windowsにはしっかり「コンテキストメニュー」として認識されているようでした。
- windowsの標準録画で録画されなかったため。
この問題はイベント関連(＠changeとかaddEventListenerとか)を弄る前の最初期から発生していました。

よろしくお願いします。

Hiroshiba · 2023-07-05T23:20:44Z

@thiramisu 調査のご報告ありがとうございます！！！
謎の挙動になっていつつも、あと少しという印象を受けました！

デモ版のQuasarなら正しく動きそうということでしたら、もしかしたらQuasarのバージョンを上げれば解決するのかもとちょっと期待しています。
electronでだけ起こる挙動であれば厄介そうです･･･。それはこちらのプルリクエストをお借りしてブラウザ版での挙動を確かめるという手があるかもです。

また、挙動を見ると僕含め誰か何かわかるかもなので、お手数おかけしちゃうのですがプルリクエストの作成をお願いしてもよいでしょうか 🙇

thiramisu requested a review from a team as a code owner July 1, 2023 20:06

thiramisu requested review from y-chan and removed request for a team July 1, 2023 20:06

thiramisu commented Jul 1, 2023

View reviewed changes

Hiroshiba reviewed Jul 2, 2023

View reviewed changes

thiramisu mentioned this pull request Jul 2, 2023

「見た目上のテキストだけを編集」する機能の追加 #1353

Closed

7 tasks

コードレビューに対応

de26219

thiramisu mentioned this pull request Jul 2, 2023

コンテキストメニュー表示時の引数の切り分け #1365

Merged

thiramisu closed this Jul 2, 2023

thiramisu mentioned this pull request Jul 6, 2023

コンテキストメニューのVue化 #1374

Merged

thiramisu deleted the context-menu-test branch July 23, 2023 00:08

jdkfx mentioned this pull request Aug 2, 2024

辞書の単語・読み入力欄で右クリックメニューを使えるようにする #2156

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UPDATE: コンテキストメニューの内部処理の変更 #1364

UPDATE: コンテキストメニューの内部処理の変更 #1364

thiramisu commented Jul 1, 2023

thiramisu Jul 1, 2023 •

edited

Loading

Hiroshiba left a comment

Hiroshiba Jul 2, 2023

Hiroshiba Jul 2, 2023 •

edited

Loading

Hiroshiba Jul 2, 2023 •

edited

Loading

thiramisu commented Jul 2, 2023

thiramisu commented Jul 2, 2023

Hiroshiba commented Jul 2, 2023

thiramisu commented Jul 2, 2023

Hiroshiba commented Jul 2, 2023 •

edited

Loading

thiramisu commented Jul 4, 2023 •

edited

Loading

Hiroshiba commented Jul 4, 2023

thiramisu commented Jul 5, 2023 •

edited

Loading

Hiroshiba commented Jul 5, 2023 •

edited

Loading

UPDATE: コンテキストメニューの内部処理の変更 #1364

UPDATE: コンテキストメニューの内部処理の変更 #1364

Conversation

thiramisu commented Jul 1, 2023

内容

関連 Issue

その他

thiramisu Jul 1, 2023 • edited Loading

Choose a reason for hiding this comment

Hiroshiba left a comment

Choose a reason for hiding this comment

Hiroshiba Jul 2, 2023

Choose a reason for hiding this comment

Hiroshiba Jul 2, 2023 • edited Loading

Choose a reason for hiding this comment

Hiroshiba Jul 2, 2023 • edited Loading

Choose a reason for hiding this comment

thiramisu commented Jul 2, 2023

thiramisu commented Jul 2, 2023

Hiroshiba commented Jul 2, 2023

thiramisu commented Jul 2, 2023

Hiroshiba commented Jul 2, 2023 • edited Loading

thiramisu commented Jul 4, 2023 • edited Loading

Hiroshiba commented Jul 4, 2023

thiramisu commented Jul 5, 2023 • edited Loading

Hiroshiba commented Jul 5, 2023 • edited Loading

thiramisu Jul 1, 2023 •

edited

Loading

Hiroshiba Jul 2, 2023 •

edited

Loading

Hiroshiba Jul 2, 2023 •

edited

Loading

Hiroshiba commented Jul 2, 2023 •

edited

Loading

thiramisu commented Jul 4, 2023 •

edited

Loading

thiramisu commented Jul 5, 2023 •

edited

Loading

Hiroshiba commented Jul 5, 2023 •

edited

Loading