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

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

Merged

Conversation

jdkfx
Copy link
Contributor

@jdkfx jdkfx commented Jul 3, 2024

内容

  • 辞書の単語・読み入力欄で右クリックメニューを表示
    • 選択している単語の表示
    • 単語・読みの全選択
    • 単語・読みのコピー、貼り付け
    • 単語・読みの切り取り

関連 Issue

close #747

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

スクリーンショット 2024-08-09 1 48 07
2024-08-15.3.12.57.mov

その他

  • テストが通らないので調査、修正
  • 編集・追加ダイアログを閉じて別の編集・追加ダイアログを操作すると意図しない挙動をすることに対する修正

今後の課題

  • 辞書登録用の右側パネルを別のコンポーネントとして切り出し
  • 上記に伴い、対象の処理をコンポーザブルに移動し再利用可能にする

@jdkfx
Copy link
Contributor Author

jdkfx commented Jul 19, 2024

コンポーザブルを追加し、右クリックメニューに関する処理のほとんどをそちらに移動させました。
右クリックメニューを使用したいコンポーネントに対して、コンポーザブルを呼べば利用することができますが、現状では切り取りやコピー、貼り付けの際にError: nativeElの取得に失敗しました。となってしまうため、その原因を探る必要があります。

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.

ちょっとこっちでも調べてみたんですが、なぜnativeElがundefinedになるのかよくわかんないですね・・・・・・。
もしかしたらQInputがv-model指定されている状態だと、refで得られるものがQInputじゃなくて直接HTMLInputになってるかも・・・?

なのでconst nativeEl = this.textfield.value?.nativeElとなっている部分をconst nativeEl = this.textfield.valueにすればとりあえず動きました・・・。
何か参考になれば 🙇

@jdkfx
Copy link
Contributor Author

jdkfx commented Jul 22, 2024

@Hiroshiba
ありがとうございます!!
nativeElの部分を変えてみてどういう挙動をするのか見つつ、実装をまた進めていきます!!

@Hiroshiba
Copy link
Member

設計方針についてコメントです 🙏

現状qInputRefinputElement(実質text)が渡されていると思います。
これを渡しているのはOKなのですが、後者はtextという名前にした方がいいと思います。
これで役割がはっきりするはず。

で、たぶんuseRightClickContextMenu内でHTMLInputElementを取得してるのはなくせると思います。
(つまりhandleFocusがいらないはず。)
HTMLInputElementを介さず、textのrefから直接テキストを得たりテキストを代入したりすれば良いはず。

あとsetTextは返り値になくても良いかも。
@update:modelValue="setSurfaceText"としてるけど、たぶんこれなしでそのままv-model使ってOKなはず。
(もしかしたらバグるかもだけど)

だいぶVueに慣れている自分からのコメントなので、結構説明が短絡的になってるかもです。
またわからないことがあれば何でも聞いてください!!

@jdkfx
Copy link
Contributor Author

jdkfx commented Aug 2, 2024

@Hiroshiba
修正を行い、切り取り・コピーペーストの処理を実装することができましたので、一旦Draftを外そうと思います。
しかし、 #1364 (comment) にもあるように、inputのフォーカスが変わると選択しているテキストのコピーペーストが意図せぬ挙動を起こしてしまいます。
そのために、以下のように既存の実装(src/components/Talk/AudioCell.vue)をこちらに持ってきてみようと思っているのですが、ご意見をいただきたいです。

const willFocusOrBlur = ref(false);
const {
  contextMenu: surfaceContextMenu,
  contextMenuHeader: surfaceContextMenuHeader,
  contextMenudata: surfaceContextMenudata,
  readyForContextMenu: readyForSurfaceContextMenu,
  clearInputSelection: clearSurfaceInputSelection,
  startContextMenuOperation: startSurfaceContextMenuOperation,
  endContextMenuOperation: endSurfaceContextMenuOperation,
} = useRightClickContextMenu(
  surfaceInput,
  surface,
  ref("surface"),
  willFocusOrBlur,
);

@jdkfx jdkfx marked this pull request as ready for review August 2, 2024 08:42
@jdkfx jdkfx requested a review from a team as a code owner August 2, 2024 08:42
@jdkfx jdkfx requested review from Hiroshiba and removed request for a team August 2, 2024 08:42
@Hiroshiba
Copy link
Member

@jdkfx
変な挙動してしまうのは可能であれば回避してあげたいですね・・・!

willFocusOrBlurをコンポーザブルを使う側で定義してコンポーザブルに渡すの、良いと思います!
ただuseRightClickContextMenuの実装次第では、もしかしたらコンポーザブルの中で定義してあげて外に出す、あるいは外には出さない設計もできるかも・・・?
ちょっとこれは実際に実装してみないとわからない&見てみないと分からないので、一旦何らかの方法で作ってみるというのはどうでしょうか・・・!

@jdkfx jdkfx force-pushed the feature/#747_right_click_menu_in_dictionary branch from f8763b7 to 3ce1f1f Compare August 5, 2024 14:57
@jdkfx
Copy link
Contributor Author

jdkfx commented Aug 5, 2024

@Hiroshiba
とりあえずで書いてみたものなのですが、変な挙動をしている状態のままになっています。すみません!
問題がどこにあるのか、何が問題なのか、どう解決すべきなのかということについてなかなか思うように解決の糸口を探ることができませんでした!

@Hiroshiba
Copy link
Member

Hiroshiba commented Aug 6, 2024

お! 現象としてどういうことが起こってる感じでしょうか 👀
こうなっていて欲しいけどなっていない、という点をご教示いただければ・・・!

@jdkfx
Copy link
Contributor Author

jdkfx commented Aug 6, 2024

@Hiroshiba

2024-08-07.2.07.57.mov

言葉での説明が難しいので、キャプチャを用いて説明します。

  • 辞書単語の「琴葉葵/コトノハアオイ」の"アオイ"部分をコンテキストメニューからコピーを選択します。ここで"アオイ"という文字列がクリップボードに保存されていると考えられます。
  • コピーした"アオイ"を単語のインプットの末尾に貼り付けます。貼り付けた結果、単語部分は"琴葉葵アオイ"となり、これは想定されている動作です。
  • 辞書単語の「琴葉葵アオイ/コトノハアオイ」を保存する前に、読みの"コトノハアオイ"をコピーしておきます。ここで"コトノハアオイ"という文字列がクリップボードに保存されていると考えられます。「琴葉葵アオイ/コトノハアオイ」を保存します。
  • 辞書単語の「琴葉茜/コトノハアカネ」を選択し、編集を行います。
  • 辞書単語「琴葉茜/コトノハアカネ」の、単語部分の末尾にカーソルを設置し、コンテキストメニューを開き、貼り付けを行います。貼り付けた結果、単語部分は"琴葉葵アオイコトノハアオイ"となってしまいます。ここで想定される単語部分の貼り付け結果は"琴葉茜コトノハアオイ"であることが望ましいため、この部分で意図しない挙動を起こしています。

過去にコピー貼り付け動作を行った内容が、別の単語読み辞書でも反映されてしまうといえばいいのかなと考えています。

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.

詳しくありがとうございます!!!

見てみました!
実際にどういうコードを書いたのか↓でプルリクエストを出してみました!

コード共有の意図なので適当にクローズしていただければ 🙏


原因はなかなか難しいのですが、こういう感じです!

  • Vueのv-ifはon/offになるたびにDOMを作り直す
  • 辞書登録用の右側のパネルはv-ifで表示を制御してる
  • なのでパネルの中にあるsurfaceInputなどは、開いたり閉じたりするたびにDOMが再構成されてる
  • QInputSelectorは辞書ダイアログが開かれた時に1回だけ呼ばれる
  • その後初めて右クリックでコンテキストメニューが開かれた時にQInputのを取得してnativeElをキャッシュする
  • キャッシュしてしまってるので、一度閉じて開いたら、すでに画面上にないQInputの選択範囲を取ろうとし、空の文字列を得る
  • 今のコードだと空の文字列の場合でも、選択している文字列を上書きしないコードになってるので、以前選択したものがそのまま現れる
  • まあそんなこんなで不可解な挙動になってる

解決策はいくつかありそうです!
その筋の良さも合わせてご紹介したいと思います。

  • v-ifを使わず、v-showにする
    • ここのコードをv-showにする
    • 要素の再生成がされなくなるので、結構正しい動きをするようになるはず
    • ただ別のところでv-ifされるようになると同じようにバグってしまうのでちょっと微妙
  • QInputSelector内でnativeElのキャッシュを取らないようにする
    • 消えてしまったQInputのキャッシュを持ち続けてしまっているのもよくない
    • なのでnativeElのキャッシュを取らないように、毎回現在のQInputから取得するようにすればうまく動くはず?(試してないですが)
    • これも微妙に根本の解決とはずれてる
  • 右側のパネルを別コンポーネントにする
    • そもそも問題なのは、v-ifで消えたり現れたりするものを、コンポーネントとして切り分けずダイアログ全体で管理してること
    • v-ifで聞いたり現れたりするもの全部を別コンポーネントに分ければ良い
    • そのコンポーネントの中でuseRightClickContextMenuを使う
    • そうするとDOMが再度現れるたびにuseRightClickContextMenuが実行されるようになる
    • 多分これが一番綺麗・・・というかコンポーネントで分けていく思想に合ってるんだけど、Vueに慣れていないとちょっと大変

という感じです!!

これは提案なのですが、もしVueなどのWeb UIフレームワークに慣れることが目的なのでしたら、このコンポーネント切り出しに挑戦してみるのも勉強になると思います!!
コンポーネント側に単語や読みの情報を渡したり、逆にコンポーネント側からイベントをemitしてもらったりする必要があるので結構大変ではあるかもですが、どれもそこそこ基本的なことなので勉強になるかなと!
その時はコンポーネントを切り出すだけの別のプルリクエストを作っていただけるとちょっとありがたいです!

もし時間がなければ他の方法でも、あるいは他のissueにチャレンジしてみるのもありだと思います!
また困ったことがあったら何でも聞いてください!!

@jdkfx
Copy link
Contributor Author

jdkfx commented Aug 7, 2024

@Hiroshiba
Vueに慣れていない部分もあり、わからないことが多いため、詳しい解説をしてくださり本当にありがとうございます!!!!
いただいたレビュー内容をもとに再度修正を行ってみます!!!

もしVueなどのWeb UIフレームワークに慣れることが目的なのでしたら、このコンポーネント切り出しに挑戦してみるのも勉強になると思います!!

ちょうど、詰まってしまったので気分転換にやっていたコンポーネント切り出しのIssueがうまくいきそうでしたので、こちらも併せてどこかでPRを出せればいいかなと思っています!

@jdkfx
Copy link
Contributor Author

jdkfx commented Aug 14, 2024

@Hiroshiba
諸々の修正を行いました!また、動いている動画を最新のものにしています!

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.

変更ありがとうございます! レビューしました!!

1点ちょっと設計を迷ってる点を。

今QInput1つとuseRightClickContextMenu1つの2つを定義して、関数とかを代入してますよね。
これらをまとめちゃって1つの新しいコンポーネントを作った方がいいのか分からないでいます。
気軽に右クリックコンテキストメニューが使えるInputコンポーネントが欲しい気がするので、まとめたものも用意すると便利なのかなぁ・・・みたいな。
(少なくともこのプルリクエストは今の実装で問題ないはず。新しくコンポーネントを作るべきかどうかという観点です。)

src/composables/useRightClickContextMenu.ts Outdated Show resolved Hide resolved
src/composables/useRightClickContextMenu.ts Outdated Show resolved Hide resolved
src/composables/useRightClickContextMenu.ts Outdated Show resolved Hide resolved
src/composables/useRightClickContextMenu.ts Show resolved Hide resolved
@jdkfx
Copy link
Contributor Author

jdkfx commented Aug 15, 2024

気軽に右クリックコンテキストメニューが使えるInputコンポーネントが欲しい気がするので、まとめたものも用意すると便利なのかなぁ・・・みたいな。

確かに言われてみれば便利そうな気がします!!
調べてないので確実なことは言えませんが、コンテキストメニューを使用している箇所で、contextMenudataの中身が違うところがあった気がしたので、その部分の制御とかもできるといいなと思いました。

@jdkfx jdkfx force-pushed the feature/#747_right_click_menu_in_dictionary branch from d738f95 to 0680595 Compare August 16, 2024 16:11
@jdkfx jdkfx requested a review from Hiroshiba August 17, 2024 09:38
@jdkfx
Copy link
Contributor Author

jdkfx commented Aug 17, 2024

@Hiroshiba
SelectionHelperForQInputの変更を追加しました。
nativeElを常に新しく取得し直すようにしています。辞書ページの開閉を繰り返してもテキストの範囲選択に問題が起きないようにしました。

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/composables/useRightClickContextMenu.ts Outdated Show resolved Hide resolved
src/composables/useRightClickContextMenu.ts Outdated Show resolved Hide resolved
src/composables/useRightClickContextMenu.ts Outdated Show resolved Hide resolved
src/composables/useRightClickContextMenu.ts Outdated Show resolved Hide resolved
src/composables/useRightClickContextMenu.ts Outdated Show resolved Hide resolved
src/composables/useRightClickContextMenu.ts Outdated Show resolved Hide resolved
@jdkfx
Copy link
Contributor Author

jdkfx commented Aug 18, 2024

@Hiroshiba
諸々細かい修正を行いました!

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

根気よく付き合ってくださってありがとうございました!!
しっかりしたコンポーザブルができたと思います!!

もしよかったらぜひまた要素切り出しにチャレンジしてみてください!!
いつもながらですが、不明な点があればissueとかでいつでもお聞きください 🙏

src/helpers/SelectionHelperForQInput.ts Outdated Show resolved Hide resolved
src/composables/useRightClickContextMenu.ts Outdated Show resolved Hide resolved
src/composables/useRightClickContextMenu.ts Outdated Show resolved Hide resolved
src/composables/useRightClickContextMenu.ts Outdated Show resolved Hide resolved
@Hiroshiba Hiroshiba merged commit e640cd2 into VOICEVOX:main Aug 19, 2024
9 checks passed
@jdkfx
Copy link
Contributor Author

jdkfx commented Aug 19, 2024

@Hiroshiba
こちらこそ不明点の解消やアドバイスなどありがとうございました!
こちらのPRに関するIssueもいくつか立てさせていただきます!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants