-
Notifications
You must be signed in to change notification settings - Fork 312
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
再生位置指定機能と、再生位置フォーカス・スクロール機能の追加 #578
再生位置指定機能と、再生位置フォーカス・スクロール機能の追加 #578
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
議論の余地がある点をコメントしてみました。
UI触ってみました!すごく良いですね!!
あと実装なのですが、おそらくこの「アクセント句の選択」状態は今後もずっと使っていくことになると思うので、ユビキタス言語を何にするかも考えたいかもです。 |
そうですね、この挙動は不自然なので、修正します。
こちらも対応します。
実験的ですが、少し実装してみました。 2021-12-16.14-59-48.mp4こちら、アクセント句が一部でも映っていれば、スクロールをしないようにしてみたのですが、どうでしょうか...
確かに、私の実装で新たなユビキタス言語が増えてしまっていましたね... |
ページめくり良いですね!! アニメーションを見たときの疲れを最小化するのが最も良いと思っていて、どうすれば良いのかを考えてました。 あと見た目が全く変わらないという選択もできるように、スクロールのON/OFF機能はぜひほしいなと感じました。 とはいえ、一旦実装してから考えるのがOSS的に良いのかなと思います! 個人的にこのプルリクエストでは、こんな感じの実装になっていればよいのかなと感じています。
あと、追加でこうしたほうが良さそうと思ったのですが、連続再生時はAccentPhraseの左側の位置を固定ではなく、中心の位置を固定できると嬉しいかもです。 プルリクエストの範囲をどこまでにするかはお任せします・・・!
良いと思います! |
縮尺の変更は破壊的変更になりそうで、例えば縮尺を開くすると操作UIが小さくなりUXが下がることが見込まれ、私には実装も難しそうなので、要議論な気がしました。 AccentPhraseが必ず左に来るようにするのは、仰る通りUIの崩壊が起きる気がしていて、私にもパッときれいに実装する方法が思い浮かばないので、妥協案として現状のままで行きたいというのが私の今の考えです。
個人的には、最後にこの状態になればいいかなと思っています(ユーザーにとっては選択肢が多い方がいろいろな使い方ができると思うので)
こちら、以下のように実装しました(現在はコメントアウトしており、ページめくりが有効化されています)。 2021-12-17.11-02-37.mp4完全なセンタリング(端のアクセント句は除く)になったので、結構動きが少し少なくなって見やすくなった気がします。
確かにそう思ったのですが、では読点(
現状、 |
要望いただいたこともあるし、必要そうな気がしているのでissue化したいかなと思いました・・・!
たしかにフォーカスしている箇所が即左に表示されるのは良くないと思います。ページ送りする際に一番左に来るイメージでした。
たしかに・・・ |
なるほど、では別問題として扱うということで、ここでは考えないこととします。
すみません、最初からそう書いてあったのに文意が読み取れてなかったですね...
これに関しては、読点の左右どちらでもアクセント句の結合操作ができることがまず問題な気がしています。 |
なるほどです、確かに読点の右の隙間(アクセント区の右端)が含まれなければOKな気がしました!そちらでお願いします! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
レビューが遅れて申し訳ありません…。
気になった点をコメントしました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ちょっとまだ全部見れていないのですが、コメントしてみました!!
…selectable-play-point
本PR内で、挙がっていた問題の修正、設定画面の追加までやってしまいました。 2022-01-19.03-26-41.mp4P.S. 加えて、アクセント句追従の間隔を0.1秒から0.01秒に変更しました。 |
…play-point # Conflicts: # src/components/AudioDetail.vue
src/components/AudioDetail.vue
Outdated
}); | ||
}; | ||
|
||
// 再生開始位置・話速変更時に、play offsetを更新する |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
いろんなとこでsetPlayOffsetが必要になってて、ステートの変更がややこしくなってそうだなと感じました。
将来この関数を呼ぶべきタイミングで呼び忘れてしまうことがそこそこ想像できそうです。
startPointをvuex側で持っておいてplayOffsetを再生時のタイミングで計算するようにすると綺麗になるかもです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
将来的な改変(音素単位等での再生位置指定)のことを考えてaudioPlayOffset
のみをVuexに持たせていましたが、もはやその必要もなさそうな気がするので、仰る通り再生開始アクセント位置をVuexに持たせ、再生する度に算出するのが綺麗そうですね...
その方向で修正してみます。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あ、仮にそうなった場合はおそらくstartPointに音素単位の再生位置情報が含まれると思います。
であれば、startPointをVuexに持たせておけば自然な拡張でaudioPlayOffset
を計算できるかもです・・・!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
audioPlayOffset
をstore内で持つ必要がなくなったこと(将来的な目的で残してもいいが、冗長になる)から、audioPlayStartPoint
に変更し、accentPhraseOffsets
をGET_AUDIO_PLAY_OFFSETS
actionとしてVuexに移動させました。
若干気持ち悪さが残りますが、私的にはこれ以上綺麗に書けそうになかったです....
1dbb197
to
4d7d0ba
Compare
…play-point # Conflicts: # src/components/SettingDialog.vue
頂いた指摘の改善をしてみました。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
1つだけ、修正してもしなくてもというコメントをしてみました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
すみません・・・。
試しに利用していたときに気づいたことを1つだけ修正お願いしたいです。
途中から再生し始めたとき、一瞬だけ1つ前のアクセントフレーズが光りそうです。
https://user-images.githubusercontent.com/4987327/150687927-65697741-8295-406e-b275-8d1f37383e4b.mp4
たぶんユーザーさんから目が疲れる的な指摘をもらいそうなので、なんとかして防ぎたく。。
調べてみたところ、currentTimeをセットした後、再生時に小さい値が切り捨てられることによって少し時間が戻るということが起きているようでした。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
素早い修正ありがとうございます!!
何回もリクエストをお願いしてすみません。。
よくあるページ送りの実装は、見えていない部分があるときに先にスクロールする感じになっていると思うので、アクセント句のうち見えないところがあったらスクロールという処理に変えて頂けると・・・。。
対応しました...! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!!
需要の有りそうな2つの機能の実装、ありがとうございました!!!
内容
題の通り
関連 Issue
close #491
スクリーンショット・動画など
2021-12-13.01-13-34.mp4
その他
watchを使っている関係上、開発時点でのホットリロードが入るとバグります。現状の私には解決策がわからなかったです...