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

Update: キー割り当て設定画面のUIを改善 #1511

Closed
wants to merge 14 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
270 changes: 159 additions & 111 deletions src/components/HotkeySettingDialog.vue
Copy link
Member

Choose a reason for hiding this comment

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

最大横幅を制限し、横長な画面でもみやすく

とりあえず両方とも左揃えにして、こんな感じになりました。

デザインに関してですが、右側が空いているUIはかなり見慣れないので、両端が空いているUIか、右側を何かしらで埋めたUIにすると良いのかなと思っています。
ツールバーは左寄せのままで良いと思います。

もし変更お願いできると嬉しいですが、難しそうであればこちらでやっちゃおうかなと思います 🙇
(個人的には真ん中寄せでとりあえず良いかなと思ってます。何がダメなのかわからなくて振動するので、とりあえず最も責任が大きい自分の考えを採用しようかなと思っています。。)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

これについて、根本原因は別な気がしたので、それに関してディスカッションに書き込みしました。
#1564 (comment)

自分はダイアログであるとはあまり認識できず、画面であるイメージが強かったです。
変数名ではDialogとされているにもかかわらず、自分にはダイアログっぽく見えないことについてをまずもっと考えるべきだったかもです…。
デザイン的にもそうなんですけど、UX的に良くなさそうという意味合いも強いです。

自分には画面だと見えている以上、ちょっとダイアログとして見た目はどうかという議論には責任が持てないですね…それでも大丈夫なら中央寄せにしようかと思います。

Copy link
Member

@Hiroshiba Hiroshiba Sep 30, 2023

Choose a reason for hiding this comment

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

issue作成ありがとうございます!

ショートカットキーのテーブルを中央寄せするかどうかに関しては、ダイアログなのか画面なのかは意識してませんでした。

  • 他と見た目を統一させておきたい
  • キーとバリューは近く表示したい(横幅の最大長を固定にしたい)
  • よく見かけるUIにしたい

を全部満たすにはまあテーブル中央寄せかなぁと。少なくとも問題は無いと思います!

そもそも若干イケてない感は僕も感じているので、どっちかというと抜本的な解決策を思いつく部分を土俵にしたい感があります。

Copy link
Member

@Hiroshiba Hiroshiba Sep 30, 2023

Choose a reason for hiding this comment

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

開いたときに現在の設定が表示されるように

で、入力中だというのが分かりやすいのと引き換えに何の変更中かが分かりにくくなっている気がします。
2つ解決策を考えてみました。

  1. 強調表示を維持する
  2. 説明を加える

任意のテキストがダイアログのタイトルに表示されるのはやはりちょっと避けるべきだと思います。
極端な例ですが、将来「ダイアログを閉じる」というショートカットキーが現れた時に、OKを選べばいいのかキャンセルを選べばいいのかわからなくなります。
(説明は書いても読まれないと想定してUIを作るのがいいかなと)

強調表示を維持するのはすごく良いと思いました!!
ショートカットダイアログ内に書くのはやめて、強調表示を目指すか、あるいはまた別の機会に抜本的な解決策を考えるというのはどうでしょう。
(そもそもダイアログを開かないとか。)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

とりあえず強調表示を試してみようかと思います。
時間が開いてしまったので念のため確認なんですが、下記はなかったこととして大丈夫ですかね。

2つ解決策を考えてみました。
説明を加える

まあ確かに書いていた方が親切だなと思い直しました。説明を加える形がいいのかなと!

Copy link
Member

Choose a reason for hiding this comment

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

あ、大丈夫です! 🙇

二転三転してしまっていて申し訳ないです。
今回のことはかなり反省しています。
せっかくプルリクエストを頂いたんだから採用したいという気持ちと、でも自分は前のほうが良いと思っているという気持ちが両方ある中で、複数の議論が同時進行して判断基準がぶれてしまった形でした。
議論が複数起こる場合、議論内容をなるべく独立させて論点と結論が見返しやすい形を目指していこうと思います。

ガシガシUI/UXを改善できる方は経験上珍しいので、 @thiramisu さんのPRやissueはとても助かっています。
未熟者なのでご迷惑おかけしてしまうこともあるかもなのですが、これからももしよければどんどん提案頂けると嬉しいです 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

いえ、こちらこそ、自分が甘かった故に意思決定のプロセスを乱してしまってすみません🙇
未熟で申し訳ないです。

了解です!

Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,10 @@
</template>

<template #body="tableProps">
<q-tr :props="tableProps">
<q-tr
:props="tableProps"
@click="openHotkeyDialog(tableProps.row.action)"
Copy link
Contributor Author

@thiramisu thiramisu Aug 19, 2023

Choose a reason for hiding this comment

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

行全体をダイアログ表示のための当たり判定にしました

>
<q-td
:key="tableProps.cols[0].name"
no-hover
Expand Down Expand Up @@ -104,19 +107,35 @@
})
.join(' + ')
"
@click="openHotkeyDialog(tableProps.row.action)"
/>
<q-btn
rounded
flat
icon="settings_backup_restore"
padding="none sm"
size="1em"
:disable="checkHotkeyReadonly(tableProps.row.action)"
@click="resetHotkey(tableProps.row.action)"
>
<q-tooltip :delay="500">デフォルトに戻す</q-tooltip>
</q-btn>
<div class="buttons">
<q-btn
rounded
flat
icon="settings_backup_restore"
padding="none sm"
size="1em"
:disable="checkHotkeyReadonly(tableProps.row.action)"
@click.stop="
Copy link
Contributor Author

Choose a reason for hiding this comment

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

行全体を当たり判定にしたので、伝播しないようにstopを追加しました

resetHotkeyWithConfirm(tableProps.row.action)
"
>
<q-tooltip :delay="500">デフォルトに戻す</q-tooltip>
</q-btn>
<q-btn
rounded
flat
icon="delete"
padding="none sm"
size="1em"
:disable="checkHotkeyReadonly(tableProps.row.action)"
@click.stop="
deleteHotkeyWithConfirm(tableProps.row.action)
"
>
<q-tooltip :delay="500">未割り当てにする</q-tooltip>
</q-btn>
</div>
</q-td>
</q-tr>
</template>
Expand All @@ -135,31 +154,37 @@
@update:model-value="closeHotkeyDialog"
>
<q-card class="q-py-sm q-px-md">
<q-card-section align="center">
<div class="text-h6">ショートカットキーを入力してください</div>
</q-card-section>
<q-card-section align="center">
<template v-for="(hotkey, index) in lastRecord.split(' ')" :key="index">
<q-card-section>
<template
v-for="(hotkey, index) in targetRecord.split(' ')"
:key="index"
>
<span v-if="index !== 0"> + </span>
<!--
Mac の Meta キーは Cmd キーであるため、Meta の表示名を Cmd に置換する
Windows PC では Meta キーは Windows キーだが、使用頻度低と考えられるため暫定的に Mac 対応のみを考慮している
-->
<q-chip :ripple="false" color="surface">
{{ hotkey === "Meta" ? "Cmd" : hotkey }}
{{
hotkey === "Meta"
? "Cmd"
: hotkey === ""
? EMPTY_COMBO_NAME
Copy link
Contributor Author

Choose a reason for hiding this comment

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

今までは空文字列は未入力を示していましたが、未入力状態でも今までの設定が表示されるようにし、空文字列は未割り当てを表すようになったので、それによる変更です。

Copy link
Member

Choose a reason for hiding this comment

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

この変更により、入力状態にも見える初期状態でなぜかOKボタンが押せないという挙動になっちゃったかもです。
(以前は空欄だったのでOKボタンが押せないのは納得感があった)

今の設定値が何かわかる一方で、なぜかOkボタンを押せないという一長一短なUXなため、どっちに倒すべきかちょっと迷ってます。
どちらも叶えられるとすごく良さそうかなと!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

初期状態は空欄として、それとは別に下の方に小さめに『以前の設定:「Ctrl + S」』みたいに表示する感じはどうでしょうか?

Copy link
Member

Choose a reason for hiding this comment

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

ありな気がしました! が、まあなくても良いかもとも思いました。
どちらでも!

Copy link
Contributor Author

@thiramisu thiramisu Oct 8, 2023

Choose a reason for hiding this comment

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

image
イメージとしてはこんな感じです。
ただ、自分はあった方が便利だと思っているんですが、一般的にはあんまり見ない気もするんですよね…
需要がないんでしょうか、それなら無くても良いかもです。

ちなみに自分があった方が便利だと思うのは、ショートカットキーは結構無意識に染みついているので、いざ設定を変えるとなると以前の設定を忘れがちだからですね。
同系統の情報かと思うんですが、「デフォルト」は、他の人に操作を伝えたい場合(あとissueで説明するときとか)に「デフォルトに戻す」をせずに参照したいことがあったので追加してみました。

Copy link
Member

Choose a reason for hiding this comment

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

どちらかというとあったほうが良さそうだとは思います。
まあかなり細部なので、自転車置き場の議論っぽみがあるなという気は若干しています。

このUIに関しては↓の形が良いかもです

  • 「デフォルト」は意外と難しい単語なので「初期設定」
  • ショートカットキー入力は左端が丸くてどうしても縦揃えにならないから中央表示
  • 「以前の設定」とかはなんとなくもうちょっと中央寄りが良さそう(だけど可変長なので難しそう)

あと、ショートカットキー入力の下に書く形であれば、今何の操作のショートカットキーを編集中なのか書いてもまあ変な伝わり方はしないのかなとちょっと思いました!
「対象の操作:ほげほげ」みたいな。(「対象の操作」という言い方はもっと良いのがありそう)

: hotkey
}}
</q-chip>
</template>
<span v-if="lastRecord !== '' && confirmBtnEnabled"> +</span>
<span v-if="isOnlyModifierKey"> +</span>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

名前を変えただけで、挙動には変更ありません。

<div v-if="duplicatedHotkey != undefined" class="text-warning q-mt-lg">
<div class="text-warning">
ショートカットキーが次の操作と重複しています
ショートカットキーが次の操作と重複しています
</div>
<div class="q-mt-sm text-weight-bold text-warning">
「{{ duplicatedHotkey.action }}」
</div>
</div>
</q-card-section>
<q-card-actions align="center">
<q-card-actions align="right">
<q-btn
padding="xs md"
label="キャンセル"
Expand All @@ -169,18 +194,6 @@
class="q-mt-sm"
@click="closeHotkeyDialog"
/>
<q-btn
padding="xs md"
label="ショートカットキーを未設定にする"
unelevated
color="surface"
text-color="display"
class="q-mt-sm"
@click="
deleteHotkey(lastAction);
closeHotkeyDialog();
"
/>
<q-btn
v-if="duplicatedHotkey == undefined"
padding="xs md"
Expand All @@ -189,10 +202,10 @@
color="primary"
text-color="display-on-primary"
class="q-mt-sm"
:disabled="confirmBtnEnabled"
:disabled="confirmBtnDisabled"
@click="
changeHotkeySettings(lastAction, lastRecord).then(() =>
closeHotkeyDialog()
changeHotkeySettings(targetAction, targetRecord).then(
closeHotkeyDialog
)
"
/>
Expand All @@ -204,8 +217,8 @@
color="primary"
text-color="display-on-primary"
class="q-mt-sm"
:disabled="confirmBtnEnabled"
@click="solveDuplicated().then(() => closeHotkeyDialog())"
:disabled="confirmBtnDisabled"
@click="solveDuplicated().then(closeHotkeyDialog)"
/>
</q-card-actions>
</q-card>
Expand All @@ -216,7 +229,7 @@
import { computed, ref } from "vue";
import { useStore } from "@/store";
import { parseCombo } from "@/store/setting";
import { HotkeyAction, HotkeySetting } from "@/type/preload";
import { HotkeySetting } from "@/type/preload";

const props =
defineProps<{
Expand All @@ -234,8 +247,6 @@ const hotkeySettingDialogOpenComputed = computed({
set: (val) => emit("update:modelValue", val),
});

const isHotkeyDialogOpened = ref(false);

const hotkeyPagination = ref({ rowsPerPage: 0 });
const hotkeyFilter = ref("");

Expand Down Expand Up @@ -264,45 +275,25 @@ const hotkeyColumns = ref<
},
]);

const lastAction = ref("");
const lastRecord = ref("");

const recordCombination = (event: KeyboardEvent) => {
if (!isHotkeyDialogOpened.value) {
return;
} else {
const recordedCombo = parseCombo(event);
lastRecord.value = recordedCombo;
event.preventDefault();
}
};
Comment on lines -267 to -278
Copy link
Contributor Author

Choose a reason for hiding this comment

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

個別設定ダイアログ関連の処理は下の方に移動しました。

// FIXME: actionはHotkeyAction型にすべき
const changeHotkeySettings = (action: string, combination: string) => {
// FIXME: しないよりはマシなので存在チェック
const _action = findHotkeySetting(hotkeySettings.value, action).action;
Copy link
Contributor Author

@thiramisu thiramisu Aug 19, 2023

Choose a reason for hiding this comment

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

応急処置ですがasよりはマシそうなので、hotkeySettingに含まれるかfindする(なかったらthrowされる)形にしました。


const changeHotkeySettings = (action: string, combo: string) => {
return store.dispatch("SET_HOTKEY_SETTINGS", {
data: {
action: action as HotkeyAction,
combination: combo,
action: _action,
combination,
},
});
};

const duplicatedHotkey = computed(() => {
if (lastRecord.value == "") return undefined;
return hotkeySettings.value.find(
(item) =>
item.combination == lastRecord.value && item.action != lastAction.value
);
});
Comment on lines -289 to -295
Copy link
Contributor Author

Choose a reason for hiding this comment

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

個別設定ダイアログ関連の処理は下の方に移動しました。


// FIXME: actionはHotkeyAction型にすべき
const deleteHotkey = (action: string) => {
changeHotkeySettings(action, "");
};
Comment on lines -297 to -300
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ボタンが2箇所に分かれた影響で、deleteHotkeyWithConfirmになりました。


const EMPTY_COMBO_NAME = "(未割り当て)";
const getHotkeyText = (action: string, combo: string) => {
if (checkHotkeyReadonly(action)) combo = "(読み取り専用)" + combo;
if (combo == "") return "未設定";
else return combo;
if (checkHotkeyReadonly(action)) {
combo = "(読み取り専用) " + combo;
}
return combo == "" ? EMPTY_COMBO_NAME : combo;
};

// for later developers, in case anyone wants to add a readonly hotkey
Expand All @@ -318,54 +309,101 @@ const checkHotkeyReadonly = (action: string) => {
return flag;
};

const findHotkeySetting = (hotkeySettings: HotkeySetting[], action: string) => {
const hotkeySetting = hotkeySettings.find(
(hotkeySetting) => hotkeySetting.action === action
);
if (hotkeySetting === undefined)
throw new Error(`「${action}」は存在しないHotkey Actionです。`);
return hotkeySetting;
};
const getCurrentHotkeyCombination = (action: string) =>
findHotkeySetting(hotkeySettings.value, action).combination;
const getDefaultHotkeyCombination = async (action: string) =>
findHotkeySetting(await window.electron.getDefaultHotkeySettings(), action)
.combination;

const getDuplicatedHotkey = (action: string, combination: string) =>
hotkeySettings.value.find(
(item) => item.combination === combination && item.action !== action
);

const resetHotkeyWithConfirm = async (action: string) => {
const result = await store.dispatch("SHOW_CONFIRM_DIALOG", {
title: "ショートカットキーの初期化",
message: `「${action}」のショートカットキーを初期値に戻します。<br>本当に戻しますか?`,
html: true,
actionName: "初期値に戻す",
});
if (result === "OK") {
const combination = await getDefaultHotkeyCombination(action);
changeHotkeySettings(action, combination);
}
};

const deleteHotkeyWithConfirm = async (action: string) => {
const result = await store.dispatch("SHOW_CONFIRM_DIALOG", {
title: "ショートカットキーの割り当ての解除",
message: `「${action}」のショートカットキーを未割り当てにします。<br>本当に解除しますか?`,
html: true,
actionName: "解除する",
});
if (result === "OK") {
changeHotkeySettings(action, "");
}
};

// 個別設定ダイアログ
const isHotkeyDialogOpened = ref(false);
const targetAction = ref("");
const targetRecord = ref("");

const duplicatedHotkey = computed(() =>
targetRecord.value == ""
? undefined
: getDuplicatedHotkey(targetAction.value, targetRecord.value)
);

const isOnlyModifierKey = computed(() =>
["Ctrl", "Shift", "Alt", "Meta"].includes(
targetRecord.value.split(" ")[targetRecord.value.split(" ").length - 1]
)
);
const confirmBtnDisabled = computed(() => {
const isChangedRecord =
getCurrentHotkeyCombination(targetAction.value) !== targetRecord.value;
return !isChangedRecord || isOnlyModifierKey.value;
});

const recordCombination = (event: KeyboardEvent) => {
if (!isHotkeyDialogOpened.value) {
return;
} else {
const recordedCombo = parseCombo(event);
targetRecord.value = recordedCombo;
event.preventDefault();
}
};

const openHotkeyDialog = (action: string) => {
lastAction.value = action;
lastRecord.value = "";
targetAction.value = action;
targetRecord.value = getCurrentHotkeyCombination(action);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

個別ダイアログを開いた時に以前の設定を表示するようにしました。

isHotkeyDialogOpened.value = true;
document.addEventListener("keydown", recordCombination);
};

const closeHotkeyDialog = () => {
lastAction.value = "";
lastRecord.value = "";
targetAction.value = "";
targetRecord.value = "";
isHotkeyDialogOpened.value = false;
document.removeEventListener("keydown", recordCombination);
};

const solveDuplicated = () => {
const solveDuplicated = async () => {
if (duplicatedHotkey.value == undefined)
throw new Error("duplicatedHotkey.value == undefined");
deleteHotkey(duplicatedHotkey.value.action);
return changeHotkeySettings(lastAction.value, lastRecord.value);
};

const confirmBtnEnabled = computed(() => {
return (
lastRecord.value == "" ||
["Ctrl", "Shift", "Alt", "Meta"].includes(
lastRecord.value.split(" ")[lastRecord.value.split(" ").length - 1]
)
);
});

const resetHotkey = async (action: string) => {
const result = await store.dispatch("SHOW_CONFIRM_DIALOG", {
title: "ショートカットキーを初期値に戻します",
message: `${action}のショートカットキーを初期値に戻します。<br/>本当に戻しますか?`,
html: true,
actionName: "初期値に戻す",
cancel: "初期値に戻さない",
});
if (result === "OK") {
window.electron
.getDefaultHotkeySettings()
.then((defaultSettings: HotkeySetting[]) => {
const setting = defaultSettings.find((value) => value.action == action);
if (setting) {
changeHotkeySettings(action, setting.combination);
}
});
}
Comment on lines -351 to -368
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ボタンが2箇所に分かれた影響で、resetHotkeyWithConfirmになりました。

await changeHotkeySettings(duplicatedHotkey.value.action, "");
await changeHotkeySettings(targetAction.value, targetRecord.value);
Comment on lines +405 to +406
Copy link
Contributor Author

Choose a reason for hiding this comment

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

片方だけがpromiseを待たれていて不自然だったので、両方待つようにしました。

};
</script>

Expand All @@ -387,14 +425,24 @@ const resetHotkey = async (action: string) => {
> :deep(.scroll) {
overflow-y: scroll;
overflow-x: hidden;

> .q-table {
max-width: 50rem;
}
}

tbody tr {
td button:last-child {
cursor: pointer;

&:hover td::before {
// Quasarデフォルトの強調表示を流用し、hover中の行を強調表示
content: "";
}
Comment on lines +437 to +440
Copy link
Member

Choose a reason for hiding this comment

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

ワークアラウンドであることはコードからわからないので、メモを残しておいてあげるとリファクタリングの役に立つかもです!
こんな感じとかどうでしょう。

Suggested change
&:hover td::before {
// Quasarデフォルトの強調表示を流用し、hover中の行を強調表示
content: "";
}
&:hover td::before {
// Quasarデフォルトの強調表示が何故か働かないのでワークアラウンド
// Quasarデフォルトの強調表示を流用し、hover中の行を強調表示
content: "";
}

td .buttons {
float: right;
display: none;
}
&:hover td button:last-child {
&:hover td .buttons {
display: inline-flex;
color: colors.$display;
opacity: 0.5;
Expand Down
Loading