ツールバーのボタンをカスタマイズ可能にする #489

y-chan · 2021-11-17T14:59:05Z

内容

題の通りです
~~※先に #490 を見てください。~~

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

その他

メニューバーの機能をコピペしてる部分があるので、それをどこか機能関数をまとめたファイルなどにまとめたい感じがあります。

This reverts commit fd3c3d9.

y-chan · 2021-11-21T18:56:51Z

タイトルバーが上の方

タイトルバーを上に戻しました！まあプレビュー中であることはすでにわかると思うので、これで良さそうですね...!

開いている状態で「設定」ボタンを押すと閉じれる

設定ボタンなどを押したときに閉じれてしまうのは盲点でした...
これに関しては、押した時点でダイアログを開くように修正しました。
なお、ヘルプに関しては直接開けてしまうので、以下のような挙動をしてしまいます。

これに関して、解決策が2つあります。

ヘルプをそもそも表示せずに破棄確認ダイアログを出す(処理的には開いた瞬間に閉じるので見えないだけです)。
ヘルプを裏で表示して、カスタマイズ終了時はヘルプを見れるようにする。

2の挙動の方がユーザー的には好ましいのかもしれないですが、若干気持ち悪いという点があります。
どちらがいいか、これに関しては議論ができるところだと思うので、まだ適用していません。

保存の横に「デフォルトに戻す」があっても良い

少し苦戦しましたが、 #472 を元に追加しました！(backgroundで定義したデフォルト値を取ってくる方法がいまいちわからず苦戦しました、先人の方、ありがとうございます...!)

加えて、レビューには書かれていませんが、すべてのボタンを消したときの挙動を改善し、「ボタン名」をで途切れていた文章を「ボタン名」を選択中に変更しました。

また、ダークテーマ時適用時の色が変になる現象を解消しました(デフォルトテーマ時の色も若干変更しています)。
例えば、色の指定がおかしくなっており、「右に動かす」「左に動かす」等のボタンが暗闇に消えてしまっていたり、以下のようにトグルボタンが見にくくなってしまっていたりする問題です。

before	after

あと、データ構造が若干冗長(?)だったので、整理しました。これによって、一度config.jsonのtoolbarSettingを削除しないと起動しないかもしれません。レビュー中に引っかかるかもしれないので書いておきます。

Hiroshiba · 2021-11-23T08:31:15Z

開いている状態で「設定」ボタンを押すと閉じれる

うーん･･･比較的ロジックが簡単そうなので、1でお願いします！！

このプルリクエストで組み込む内容じゃないですが、そもそもダイアログを開いたときはメニューボタンを押せなくなるようにしたほうが良い気がしてきました。

y-chan · 2021-11-23T09:11:18Z

1でお願いします！！

適用しました！

そもそもダイアログを開いたときはメニューボタンを押せなくなるようにしたほうが良い

確かにそうかもしれませんね….
後でIssue化しますね…!

# Conflicts: # src/components/MenuBar.vue # src/electron/preload.ts # src/type/ipc.d.ts

y-chan · 2021-11-28T13:49:20Z

コンフリクトを解消し、#518 のおかげで、不要になった部分を削除したり、一部文言の統一をしたりしました。

Hiroshiba · 2021-12-02T12:58:34Z

GENERATE_AND_SAVE_ALL_AUDIO_WITH_DIALOGなどが共通化されて良い感じだと思いました！
ただその関係で変更行数がなかなか多くなっていて、なにか間違いがあっても気づけないようになっていそうな気がしました。

GENERATE_AND_SAVE_ALL_AUDIO_WITH_DIALOGなどのアクションを共通化する部分と、ToolBarをカスタマイズ可能にする部分に分けると変更行数を少し小さくできるのかなと感じました。
どうでしょう・・・？

y-chan · 2021-12-02T13:09:37Z

GENERATE_AND_SAVE_ALL_AUDIO_WITH_DIALOGなどのアクションを共通化する部分と、ToolBarをカスタマイズ可能にする部分に分ける

なるほど、少し切り分けてみますね

Segu-g

@Hiroshiba さんが見られたようですが全体を見て，特に気になった部分をコメントしました．
正直，あまり正しいことを言っている自信がないのでダブルチェックして頂けると助かります．．．

Segu-g · 2021-12-02T14:51:05Z

src/type/preload.d.ts

+export type ToolbarButtonsType =
+  | "連続再生"
+  | "再生"
+  | "音声書き出し"
+  | "一つだけ書き出し"
+  | "停止"
+  | "元に戻す"
+  | "やり直す"
+  | "テキスト読み込み"
+  | "空白";


識別子とテキストの分離をした方が良いと思います．

Segu-g · 2021-12-02T14:51:14Z

src/store/setting.ts

+    GET_TOOLBAR_SETTING({ commit }) {
+      const newData = window.electron.toolbarSetting();
+      newData.then((toolbarSetting) => {
+        commit("SET_TOOLBAR_SETTING", { toolbarSetting });
+      });
+    },


非同期に処理を行うActionなので，Promiseをreturnするかasync functionで書き，commitされるまで待てるようにしておきましょう．

確かに、仰る通りなのですが、他のactionが同じような形だったのであまり気にしていませんでした。
修正してみようかと思います。

Segu-g · 2021-12-02T14:51:17Z

src/store/setting.ts

+    SET_TOOLBAR_SETTING({ commit }, { data }: { data: ToolbarSetting }) {
+      const newData = window.electron.toolbarSetting(data);
+      newData.then((toolbarSetting) => {
+        commit("SET_TOOLBAR_SETTING", { toolbarSetting });
+      });
+    },


非同期に処理を行うActionなので，Promiseをreturnするかasync functionで書き，commitされるまで待てるようにしておきましょう．

Segu-g · 2021-12-02T14:51:29Z

src/components/HeaderBarCustomDialog.vue

+    // 配列の比較は出来ないので、文字列として結合したものを比較する
+    const changedOrNotFlag = computed(() => {
+      const nowSetting = store.state.toolbarSetting;
+      return toolbarButtons.value.join("") !== nowSetting.join("");
+    });
+    const defaultOrNotFlag = computed(() => {
+      return toolbarButtons.value.join("") !== defaultSetting.join("");
+    });


普通に比較してしまってもいいと思います．

a.length == b.length && a.every((e, i) => e == b[i])

おっと、everyを使う手法は気づきませんでした。
そちらの方がきれいだと思うので、そちらで書き直してみたいと思います。

Segu-g · 2021-12-02T14:51:59Z

src/components/HeaderBarCustomDialog.vue

+    const usableButtons = [
+      {
+        label: "連続再生",
+        desc: "選択されているテキスト以降のすべてのテキストを読み上げます。",
+      },
+      {
+        label: "再生",
+        desc: "選択されているテキストを読み上げます。",
+      },
+      {
+        label: "停止",
+        desc: "テキストが読み上げられているときに、それを止めます。",
+      },
+      {
+        label: "音声書き出し",
+        desc: "入力されているすべてのテキストの読み上げを音声ファイルに書き出します。",
+      },
+      {
+        label: "一つだけ書き出し",
+        desc: "選択されているテキストの読み上げを音声ファイルに書き出します。",
+      },
+      {
+        label: "元に戻す",
+        desc: "操作を一つ戻します。",
+      },
+      {
+        label: "やり直す",
+        desc: "元に戻した操作をやり直します。",
+      },
+      {
+        label: "テキスト読み込み",
+        desc: "テキストファイル(.txt)を読み込みます。",
+      },
+      {
+        label: "空白",
+        desc: "これはボタンではありません。レイアウトの調整に使います。また、実際には表示されません。",
+      },
+    ];


findが多いので識別子とテキストの分離をした方が良さそうです

Segu-g · 2021-12-02T14:54:01Z

src/components/HeaderBar.vue

+
+type ButtonContent =
+  | {
+      text: ToolbarButtonsType;


ToolButtonsTypeを表示するテキストそのものにしているため，テキストだけを変更したいときにも状態のマイグレーションが必要になってしまいます．(表示の為の要素がstateの中に混在してしまっています)．
代わりに，そのボタンの機能ごとにタグを付けて管理してはどうでしょうか？(識別子とテキストの分離)

なるほど、確かに言われてみれば、テキストだけを変更したい時を考えていませんでした。
識別子化は非常に良い解決策だと思うので、対応に当たりたいと思います。

src/components/HeaderBar.vue

Segu-g · 2021-12-02T14:54:10Z

src/components/HeaderBar.vue

+    const usableButtons: ButtonContent[] = [
+      {
+        text: "連続再生",
+        click: playContinuously,
+        disable: uiLocked,
+      },
+      {
+        text: "再生",
+        click: play,
+        disable: computed(() => !activeAudioKey.value || uiLocked.value),
+      },
+      {
+        text: "停止",
+        click: stop,
+        disable: computed(
+          () => !nowPlayingContinuously.value && !nowPlaying.value
+        ),
+      },
+      {
+        text: "音声書き出し",
+        click: generateAndSaveAllAudio,
+        disable: uiLocked,
+      },
+      {
+        text: "一つだけ書き出し",
+        click: generateAndSaveOneAudio,
+        disable: computed(() => !activeAudioKey.value || uiLocked.value),
+      },
+      {
+        text: "元に戻す",
+        click: undo,
+        disable: computed(() => !canUndo.value || uiLocked.value),
+      },
+      {
+        text: "やり直す",
+        click: redo,
+        disable: computed(() => !canRedo.value || uiLocked.value),
+      },
+      {
+        text: "テキスト読み込み",
+        click: importTextFile,
+        disable: uiLocked,
+      },
+    ];


textもcomputedにすれば，連続再生ボタンを連続再生停止の為にも使えます．

また，ツールバーに配置可能なactionの多くはHotKeyでも操作可能な処理が多いと思うので，その辺りと処理を纏めてactionを作ったり，disableをgetterで返したりするstoreを作ってしまってもいいかもしれません．
もっとも，ホットキーで使用可能な操作とツールバーで使用可能な操作が真に等しいかはもう少し議論すべきでしょうから，安易に纏めずにここに書き下しても構わないと思います．

textもcomputedにすれば

確かに、これを機に再生と停止を同じボタンにまとめてしまうのは良いと思いました。
そうすれば、上のレビューであったcontinuouslyFlagの利用をしなくて済むので、複雑度が下がりそうです。
ただ、このPR内で再生と停止をまとめてしまっていいかが微妙な気がします。

また、ボタンカスタマイズの時点で、再生と停止の両方を行えるボタンであることを示すことが難しいような気もしています。
何かいい方法はないですかね...?

ツールバーに配置可能なactionの多くはHotKeyでも操作可能な処理

actionにまとめてしまう感じのことは、WITH_DIALOG系の処理で一度行っていますが、それ以外はこれ以上まとめられないような気がしています。getterを増やすのは良いかもと思いました。ちょっと検討してみます。

ボタンカスタマイズの時点で、再生と停止の両方を行えるボタンであることを示すことが難しいような気もしています。

UIについて私は詳しくないので素人なのですが，押してみれば分かりますしこれまで使っていた人が最初見た時に混乱する程度でしょうか．特に記載を行わなくても問題はなさそうな気もしますが@Hiroshiba さんに意見を貰いたいです．

それとは別に今気づいたのですが，ContinuslyフラグをAudioStateに置くべきだと思いました．どちらにしても現在の実装ではShift+Space等の別経路で連続再生したときに，連続再生したか単体再生したかHeaderBarでは判別ができないからです．
とはいっても今の実装では，STOP_AUDIOを叩いてもSTOP_CONTINUOUSLY_AUDIOを叩いてもどちらでも停止してくれるようなので分岐を間違えてもバグにはないようです．STOP_CONTINUOUSLY_AUDIOだけ叩いていても問題にはならないのですが...

ツールバーに配置可能なactionの多くはHotKeyでも操作可能な処理

ショートカットキーは一部を除いてコンポーネントに所属しないグローバルな操作なのでショートカットキーの多くはactionとして記載でき，ツールバーにも置けるかなと思いましたがこのPRでする必要は特にないと思いました．testablityが改善しそうだったので提案しましたが忙しいならこのままでも構わないと思います．

再生と停止を同じボタンにするのは別機能なので、とりあえずこのPRではしない方針で良いと思います。
UI上もそこまで大きな問題ではないので、要望が来るまではまあ良いかなと。

別経路で連続再生したときに，連続再生したか単体再生したかHeaderBarでは判別ができない

こちら目からウロコでした。見逃していました･･･レビュー感謝です。

ボタンカスタマイズの時点で、再生と停止の両方を行えるボタンであることを示すことが難しいような気もしています。

押してみれば分かりますし

確かに、プレビュー時点で押せば表記が切り替わるみたいな挙動をさせるのは良いと思いました。

ただ、やはり再生(連続再生)と停止のボタン統合は別PRで行うべきな気がします。
一旦切り出したいところですね。

ContinuslyフラグをAudioStateに置くべき

おっと、この問題は気づきませんでした...(私がボイボにおいてほとんどホットキーを使わないので、そこまで頭が回っていませんでした...)
この問題を解決する(continuslyをstateに持っていく)ついでに、ショートカットキーの多くをactionに移行するというのはありな気がします。
ただ、一部の例外を置いておくなどはそれはそれで気持ち悪いような気もしますが....
どちらにせよ、このPRの規模が大きくなる一方な気がするので、やるならば別PRへの切り出しが必要そうです。

STOP_CONTINUOUSLY_AUDIOだけ叩いていても問題にはならないのですが...

(もうSTOP_CONTINUOUSLY_AUDIOをSTOP_AUDIOと統合してもいいんじゃないかなって個人的には思います...)

Segu-g · 2021-12-02T14:54:14Z

src/components/HeaderBar.vue

+          text: null,
+        };
+      } else {
+        return usableButtons.find((b) => b.text === button) as ButtonContent;


タグで管理し，プロパティでアクセスする方が明示的です．
(このコードだと，as ButtonContentでundefinedableを潰していますが, オブジェクトならそのプロパティが存在することを型レベルで強制できます．)

型レベルで強制できる、確かに仰る通りです。

findの使い過ぎは私も少し感じていたので、オブジェクト化を試みようと思います。

Segu-g · 2021-12-02T14:54:45Z

src/components/HeaderBarCustomDialog.vue

+                <q-radio
+                  v-if="button === '空白'"
+                  v-model="selectedButton"
+                  size="0"
+                  :val="button"
+                  :label="button"
+                  :class="
+                    (selectedButton === button
+                      ? 'radio-space-selected'
+                      : 'radio-space') + ' text-no-wrap q-mr-sm'
+                  "
+                  ><q-tooltip
+                    :delay="500"
+                    anchor="center left"
+                    self="center right"
+                    transition-show="jump-left"
+                    transition-hide="jump-right"
+                    >{{
+                      usableButtons.find((v) => v.label === button).desc
+                    }}</q-tooltip
+                  ></q-radio
+                >
+                <q-radio
+                  v-else
+                  v-model="selectedButton"
+                  size="0"
+                  :val="button"
+                  :label="button"
+                  :class="
+                    (selectedButton === button ? 'radio-selected' : 'radio') +
+                    ' text-no-wrap text-bold text-display q-mr-sm'
+                  "
+                  ><q-tooltip
+                    :delay="500"
+                    anchor="center left"
+                    self="center right"
+                    transition-show="jump-left"
+                    transition-hide="jump-right"
+                    >{{
+                      usableButtons.find((v) => v.label === button).desc
+                    }}</q-tooltip
+                  ></q-radio
+                >


空白とそれ以外のボタンの違いはclassとテキストだけのようなので，v-ifで空白を特殊化するよりもclassをButtonsに持たせたりclassの部分で特殊化する方が短くなりそうです．

三項演算子が増えそうで、ちょっとコードの可読性が落ちそうだったので手を付けていなかったのですが、仰る通りそれよりもコードを縮める方がよいかもしれません。
少し検討してみます。

MT224244

レビューしてみました。
型に関して気になったところがあったのでコメントしました。

MT224244 · 2021-12-02T01:49:47Z

src/store/type.ts

+  GENERATE_AND_SAVE_ALL_AUDIO_WITH_DIALOG: {
+    action(payload: {
+      $q: QVueGlobals;
+      saveAllResultDialog: unknown;


ここの型は typeof SaveAllResultDialog ~~か DefineComponent~~ の方がいいかもしれません。

ここをDefineComponentにしなかったのは、型の調製がとてつもなくしんどかったからです(DefineComponentに渡すgenericsの引数まで合わせないと型エラーになってしまうので)

何かいい方法があればいいのですが、うまくいきそうにないので、この旨をコメントとして残しておくべきかなとは思いました。

DefineComponentに渡すgenericsの引数まで合わせないと型エラーになってしまう

おっと…これは確認不足でした。
なるほど、であれば確かにDefineComponentは使えなさそうですね…。

MT224244 · 2021-12-02T11:11:36Z

src/components/HeaderBarCustomDialog.vue

+      defaultSetting.push(...setting);
+    });
+
+    const usableButtons = [


明示的に型を付けた方がいいかもしれません。

y-chan · 2021-12-05T13:14:33Z

ちょっと、PRの規模が大きすぎるのと、同時にレビューでの改善点もかなりあるので、細分化してPRを作り直そうかと思います。

一旦closeしたいと思うのですが、どうでしょうか...?

Hiroshiba · 2021-12-05T13:23:31Z

良いと思います！

y-chan · 2021-12-05T13:30:00Z

一旦クローズします!

y-chan added 30 commits November 14, 2021 02:03

make and use list of header buttons

1da38f2

move q-header

fd3c3d9

add temporary heaedr bar custom dialog

62a4dae

remove undefined

3cb9f21

move toolbar data to vuex store

bcd9dc5

remove preview title

0ba5a0e

add flat setting to preview card

48af645

replace header bar

2a7f87d

add toolbar custom button (doesn't have func)

08e0223

Revert "move q-header"

5588bac

This reverts commit fd3c3d9.

add style of radio button hover

131aec8

replace empty string by "空白"

73fcd3c

replace space by empty button

9886dd0

カスタム -> カスタマイズ

0ceb4ca

fix bug (toolbar doesn't displayed at startup)

ddf4b1b

use ts

0c476e9

fix bug of cannot edit toolbar

ba80040

add move left and right func to buttons

1aec854

revert remove card title

ca23ae0

add setting save button

3381622

refactor and fix miss

34b80bb

add remove func to buttons

bda7055

fix error of not found selected button

80fef2c

set overflow hidden

be52685

use quaser css

63c79ce

card actions -> toolbar

b5ceafd

specify preview toolbar height

9a85b4a

add type of toolbar button

611bd8e

add play button to toolbar's usable button list

37b2a4a

add button of exporting only one item to toolbar's usable button list

359bde4

y-chan added 2 commits November 22, 2021 03:54

change select message

023414f

set head item when close dialog

19315ca

y-chan added 2 commits November 22, 2021 04:11

add apply default setting button

0ac8477

fix test case

b6a483b

y-chan force-pushed the feature/custom-toolbar branch from 2d5a6ee to b6a483b Compare November 21, 2021 19:11

move comment

1e144f2

add help dialog close process

702bd7d

y-chan added 3 commits November 28, 2021 22:01

Merge remote-tracking branch 'upstream/main' into feature/custom-toolbar

8bb921e

# Conflicts: # src/components/MenuBar.vue # src/electron/preload.ts # src/type/ipc.d.ts

remove "本当に"

4fc2b13

refactor(remove parts no longer needed by menu bar lock)

fdd2813

Hiroshiba requested review from MT224244 and Segu-g December 1, 2021 16:19

Segu-g requested changes Dec 2, 2021

View reviewed changes

MT224244 reviewed Dec 2, 2021

View reviewed changes

y-chan changed the title ~~[FEATURE]ツールバーのボタンをカスタマイズ可能にする~~ ツールバーのボタンをカスタマイズ可能にする Dec 2, 2021

y-chan mentioned this pull request Dec 4, 2021

音声をすべて繋げて書き出す機能を追加 #549

Merged

y-chan closed this Dec 5, 2021

This was referenced Dec 5, 2021

音声保存系action実行後にdialogを出すものをstoreに移動 #559

Merged

ツールバーの情報をStoreに移動させる #561

Merged

y-chan mentioned this pull request Dec 29, 2021

ツールバーのカスタムダイアログの追加 #625

Merged

y-chan mentioned this pull request Jan 20, 2022

ツールバーのカスタマイズ可能なボタンの追加 #649

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ツールバーのボタンをカスタマイズ可能にする #489

ツールバーのボタンをカスタマイズ可能にする #489

y-chan commented Nov 17, 2021 •

edited

Loading

y-chan commented Nov 21, 2021 •

edited

Loading

Hiroshiba commented Nov 23, 2021

y-chan commented Nov 23, 2021

y-chan commented Nov 28, 2021 •

edited

Loading

Hiroshiba commented Dec 2, 2021

y-chan commented Dec 2, 2021

Segu-g left a comment

Segu-g Dec 2, 2021

Segu-g Dec 2, 2021

y-chan Dec 2, 2021

Segu-g Dec 2, 2021

Segu-g Dec 2, 2021

y-chan Dec 2, 2021

Segu-g Dec 2, 2021

Segu-g Dec 2, 2021

y-chan Dec 2, 2021

Segu-g Dec 2, 2021

y-chan Dec 2, 2021

Segu-g Dec 2, 2021 •

edited

Loading

Hiroshiba Dec 2, 2021

y-chan Dec 2, 2021

Segu-g Dec 2, 2021

y-chan Dec 2, 2021

Segu-g Dec 2, 2021

y-chan Dec 2, 2021

MT224244 left a comment

MT224244 Dec 2, 2021 •

edited

Loading

y-chan Dec 2, 2021

MT224244 Dec 3, 2021 •

edited

Loading

MT224244 Dec 2, 2021

y-chan commented Dec 5, 2021

Hiroshiba commented Dec 5, 2021

y-chan commented Dec 5, 2021

ツールバーのボタンをカスタマイズ可能にする #489

ツールバーのボタンをカスタマイズ可能にする #489

Conversation

y-chan commented Nov 17, 2021 • edited Loading

内容

関連 Issue

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

その他

y-chan commented Nov 21, 2021 • edited Loading

Hiroshiba commented Nov 23, 2021

y-chan commented Nov 23, 2021

y-chan commented Nov 28, 2021 • edited Loading

Hiroshiba commented Dec 2, 2021

y-chan commented Dec 2, 2021

Segu-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Segu-g Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MT224244 left a comment

Choose a reason for hiding this comment

MT224244 Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MT224244 Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y-chan commented Dec 5, 2021

Hiroshiba commented Dec 5, 2021

y-chan commented Dec 5, 2021

y-chan commented Nov 17, 2021 •

edited

Loading

y-chan commented Nov 21, 2021 •

edited

Loading

y-chan commented Nov 28, 2021 •

edited

Loading

Segu-g Dec 2, 2021 •

edited

Loading

MT224244 Dec 2, 2021 •

edited

Loading

MT224244 Dec 3, 2021 •

edited

Loading