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

セルの全選択 #1963

Merged
merged 8 commits into from
Apr 30, 2024
Merged

セルの全選択 #1963

merged 8 commits into from
Apr 30, 2024

Conversation

ShimagayaSatoka
Copy link
Contributor

内容

セルの全選択をショートカットでできるようにしました

関連 Issue

#1962

その他

issueではctrl+Aとなっていましたがテキストの全選択と被り、重複が許されないようだったので仮にctrl+shift+Aとしました

@ShimagayaSatoka ShimagayaSatoka requested a review from a team as a code owner March 29, 2024 15:11
@ShimagayaSatoka ShimagayaSatoka requested review from Hiroshiba and removed request for a team March 29, 2024 15:11
Copy link
Member

@sevenc-nanashi sevenc-nanashi left a comment

Choose a reason for hiding this comment

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

全選択はMenuBarの編集に足してもいいと思います。

@@ -172,6 +172,10 @@ export const defaultHotkeySettings: HotkeySettingType[] = [
action: "選択解除",
combination: HotkeyCombination("Escape"),
},
{
action: "セル全選択",
combination: HotkeyCombination(!isMac ? "Ctrl shift A" : "Meta A"),
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
combination: HotkeyCombination(!isMac ? "Ctrl shift A" : "Meta A"),
combination: HotkeyCombination(!isMac ? "Ctrl Shift A" : "Meta Shift A"),

MacはCmd+Aになってそうです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shiftは入れ忘れていました
他のメニューバーにあっても良さそうな機能といっしょにメーニューバーに追加してきます。

registerHotkeyWithCleanup({
editor: "talk",
enableInTextbox: false,
name: "セル全選択",
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

Choose a reason for hiding this comment

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

既存のnameと口調を合わせようと今確認したところ、既存のものも口調に揺れがある気がするのでそこと同時に直してきます。

@sevenc-nanashi
Copy link
Member

既存のを変更したならマイグレーション(src/backend/common/ConfigManager.ts)書かないとダメそうな気がします.

@ShimagayaSatoka
Copy link
Contributor Author

defaultHotkeySettingsに含まれているものしか書き換えていないので自動でされるマイグレーションだけで大丈夫そうです

@sevenc-nanashi
Copy link
Member

されてなさそうです

Copy link
Member

@sevenc-nanashi sevenc-nanashi left a comment

Choose a reason for hiding this comment

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

LGTM!

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/backend/common/ConfigManager.ts Outdated Show resolved Hide resolved
src/backend/common/ConfigManager.ts Outdated Show resolved Hide resolved
@Hiroshiba Hiroshiba merged commit 54be47d into VOICEVOX:main Apr 30, 2024
8 checks passed
@Hiroshiba
Copy link
Member

ちょっとctrl+Aをセル全選択にできないか挑戦してみようと思います!
#1962 (comment)

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

Successfully merging this pull request may close these issues.

3 participants