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

ショートカットキーを表示名ではなく識別子でelectron-storeに持つようにする #547

Open
y-chan opened this issue Dec 3, 2021 · 4 comments

Comments

@y-chan
Copy link
Member

y-chan commented Dec 3, 2021

内容

現状、ショートカットキーはショートカットキーの表示名でストアに保持されています。
しかし、この状態は望ましくありません。
ショートカットキーの表示名を変えたい時(ショートカットキーの表示名が誤っていた時)に、いちいちマイグレーション処理が必要です。
こういったマイグレーション処理は、バグの温床になり得ます。
また、型定義的にも非常に面倒なことになります。
なので、ショートカットキーを識別子化し、設定画面などでのみ表示名をあてがうようにするとこれらの問題を解消できるのではないかと考えます。

Pros 良くなる点

ショートカットキーの表示名を変えやすくなる

Cons 悪くなる点

新たにマイグレーション処理が必要(逆に言えば、このマイグレーション処理のみで済むとも考えられる)

実現方法

現状のデータ形式は以下の形のオブジェクトのリストです。

{
  action: string,
  combination: string,
}

これを、以下の形に変えられると良さそうです。

{
  actionIdentifier: string,
  combination: string,
}

その他

それなりに変更が必要かつ、マイグレーションが絡むので慎重に行わなければなりませんが、得られるものが少ないので比較的優先度は低めかもです。

@y-chan y-chan changed the title ショートカットキーを表示名ではなく識別子でelectron-storeに持つようにする ショートカットキーを表示名ではなく識別子でelectron-storeに持つようにする Dec 3, 2021
@Patchethium
Copy link
Contributor

That's exactly what I'm doing on my i18n branch, replacing the Japanese names with a set of language-independent identifier, adding migration. Should be finished and pushed at the end of this week.

@Segu-g
Copy link
Member

Segu-g commented Dec 3, 2021

現状のデータ形式は以下の形のオブジェクトのリストです。
...
これを、以下の形に変えられると良さそうです。

{
  actionIdentifier: string,
  combination: string,
}

現在の設定ではhotkeySettingsHotkeySetting[]ですが,順序が使われているのはUI部分だけでロジックの部分ではArrayである必要はありません.
UIの表示順序はUI部分にHotkeyActionTag[]で順序を固定して埋め込んで,hotkeySettingsRecord<HotkeyActionTag, { combination: string }>で十分だと思います.

@Segu-g
Copy link
Member

Segu-g commented Dec 4, 2021

現状のデータ形式は以下の形のオブジェクトのリストです。
...
これを、以下の形に変えられると良さそうです。

{
  actionIdentifier: string,
  combination: string,
}

現在の設定ではhotkeySettingsHotkeySetting[]ですが,順序が使われているのはUI部分だけでロジックの部分ではArrayである必要はありません. UIの表示順序はUI部分にHotkeyActionTag[]で順序を固定して埋め込んで,hotkeySettingsRecord<HotkeyActionTag, { combination: string }>で十分だと思います.

1つのcombinationに対して複数のHotkeyActionTagを設定してはいけない,という制約だとRecord<HotkeyActionTag, { combination: string }>よりもRecord<CombinationStr, HotkeyActionTag>の方が適切だと思いました

@Patchethium
Copy link
Contributor

Changing the data structure leads to much extra work, which I just don't... feel like dealing with after finishing the whole i18n feature. I'll keep the changes minimal, anyone interested can improve it later.

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

No branches or pull requests

4 participants