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.

Sign up for GitHub

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

Jump to bottom

Fix the title and text of the Discard Changes prompt #2706

Merged
merged 3 commits into from
Jun 26, 2023
Merged

Fix the title and text of the Discard Changes prompt #2706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51a5580
Fix the title and text (and variable names) of the Discard Changes pr…
stefanhaller Apr 17, 2023
ee03a0b
Rename "Delete change" menu entry to "Discard change" in staging panel
stefanhaller Jun 5, 2023
2f6b7b9
Rename the gui.skipUnstageLineWarning conf key to gui.skipDiscardChan…
stefanhaller Jun 8, 2023
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
2 changes: 1 addition & 1 deletion docs/Config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ gui:
commitLength:
show: true
mouseEvents: true
skipUnstageLineWarning: false
skipDiscardChangeWarning: false
skipStashWarning: false
showFileTree: true # for rendering changes files in a tree format
showListFooter: true # for seeing the '5 of 20' message in list panels
Expand Down
2 changes: 1 addition & 1 deletion docs/keybindings/Keybindings_en.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ _Legend: `<c-b>` means ctrl+b, `<a-b>` means alt+b, `B` means shift+b_
<kbd>&lt;esc&gt;</kbd>: Return to files panel
<kbd>&lt;tab&gt;</kbd>: Switch to other panel (staged/unstaged changes)
<kbd>&lt;space&gt;</kbd>: Toggle line staged / unstaged
<kbd>d</kbd>: Delete change (git reset)
<kbd>d</kbd>: Discard change (git reset)
<kbd>E</kbd>: Edit hunk
<kbd>c</kbd>: Commit changes
<kbd>w</kbd>: Commit changes without pre-commit hook
Expand Down
2 changes: 1 addition & 1 deletion docs/keybindings/Keybindings_pl.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ _Legend: `<c-b>` means ctrl+b, `<a-b>` means alt+b, `B` means shift+b_
<kbd>&lt;esc&gt;</kbd>: Wróć do panelu plików
<kbd>&lt;tab&gt;</kbd>: Switch to other panel (staged/unstaged changes)
<kbd>&lt;space&gt;</kbd>: Toggle line staged / unstaged
<kbd>d</kbd>: Delete change (git reset)
<kbd>d</kbd>: Discard change (git reset)
<kbd>E</kbd>: Edit hunk
<kbd>c</kbd>: Zatwierdź zmiany
<kbd>w</kbd>: Zatwierdź zmiany bez skryptu pre-commit
Expand Down
30 changes: 30 additions & 0 deletions pkg/config/app_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"

"github.com/OpenPeeDeeP/xdg"
"github.com/jesseduffield/lazygit/pkg/utils/yaml_utils"
"gopkg.in/yaml.v3"
)

Expand Down Expand Up @@ -156,6 +157,11 @@ func loadUserConfig(configFiles []string, base *UserConfig) (*UserConfig, error)
return nil, err
}

content, err = migrateUserConfig(path, content)
Copy link
Owner

Choose a reason for hiding this comment

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

I would like a way to track which 'version' of the config we're on so that we only need to run migrations if we are behind. This has a few benefits:

  • No need to enforce idempotency in migrations
  • Less frequently messing with the user's config (I suspect that due to indentation, changes the file will often be overwritten despite no semantic change)
  • Less work on startup (though I suspect it's not currently eating up much time).

We could store this version on the config itself. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think this would work. If we read a config file that doesn't have a version, there's no way to tell whether this is a new user who just created a new config file for the latest version, or if it's an old config file from before we started to put a version in the file. Unless we require the user to manually add the correct version when they initially create a config file, which I don't think we should.

I also disagree with some of the alleged downsides:

  • Less frequently messing with the user's config: on the contrary, if we put a version number in the file, we'll have to rewrite the file for every user whenever we add a new migration. If we don't version the file, we only need to rewrite it for those users who have the migrated key in their config. This requires adding a commit to the underlying PR that avoids rewriting the file when nothing changed; see Improve yaml_utils #2718 (comment).
  • Less work on startup: That's true, but I agree that it's probably not too significant.
  • So that leaves the idempotence question. I don't feel this is going to be a problem for the kind of migration that we are likely to add: renaming a key, or changing the type of a key (e.g. from a bool to an enum). Can you think of a migration that would be problematic in this regard?

Copy link
Owner

Choose a reason for hiding this comment

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

Good points. Let's go with your approach then :)

if err != nil {
return nil, err
}

if err := yaml.Unmarshal(content, base); err != nil {
return nil, fmt.Errorf("The config at `%s` couldn't be parsed, please inspect it before opening up an issue.\n%w", path, err)
}
Expand All @@ -164,6 +170,30 @@ func loadUserConfig(configFiles []string, base *UserConfig) (*UserConfig, error)
return base, nil
}

// Do any backward-compatibility migrations of things that have changed in the
// config over time; examples are renaming a key to a better name, moving a key
// from one container to another, or changing the type of a key (e.g. from bool
// to an enum).
func migrateUserConfig(path string, content []byte) ([]byte, error) {
changedContent, err := yaml_utils.RenameYamlKey(content, []string{"gui", "skipUnstageLineWarning"},
"skipDiscardChangeWarning")
if err != nil {
return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
}

// Add more migrations here...

// Write config back if changed
if string(changedContent) != string(content) {
if err := os.WriteFile(path, changedContent, 0o644); err != nil {
return nil, fmt.Errorf("Couldn't write migrated config back to `%s`: %s", path, err)
}
return changedContent, nil
}

return content, nil
}

func (c *AppConfig) GetDebug() bool {
return c.Debug
}
Expand Down
24 changes: 12 additions & 12 deletions pkg/config/user_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ type GuiConfig struct {
ScrollHeight int `yaml:"scrollHeight"`
ScrollPastBottom bool `yaml:"scrollPastBottom"`
MouseEvents bool `yaml:"mouseEvents"`
SkipUnstageLineWarning bool `yaml:"skipUnstageLineWarning"`
SkipDiscardChangeWarning bool `yaml:"skipDiscardChangeWarning"`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a breaking change, but I didn't bother doing something about it. Since the warning prompt mentions the config, it's easy enough to fix it manually.

We could however consider rewriting the config file with the renamed property, now that that's possible. Worth it?

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's worth rewriting the config. Note that the existing code in pkg/utils/yaml_utils/yaml_utils.go is not exactly what we need here: we need to replace a key rather than replace a value, but I assume it's similar enough logic

SkipStashWarning bool `yaml:"skipStashWarning"`
SidePanelWidth float64 `yaml:"sidePanelWidth"`
ExpandFocusedSidePanel bool `yaml:"expandFocusedSidePanel"`
Expand Down Expand Up @@ -397,17 +397,17 @@ type CustomCommandMenuOption struct {
func GetDefaultConfig() *UserConfig {
return &UserConfig{
Gui: GuiConfig{
ScrollHeight: 2,
ScrollPastBottom: true,
MouseEvents: true,
SkipUnstageLineWarning: false,
SkipStashWarning: false,
SidePanelWidth: 0.3333,
ExpandFocusedSidePanel: false,
MainPanelSplitMode: "flexible",
Language: "auto",
TimeFormat: "02 Jan 06",
ShortTimeFormat: time.Kitchen,
ScrollHeight: 2,
ScrollPastBottom: true,
MouseEvents: true,
SkipDiscardChangeWarning: false,
SkipStashWarning: false,
SidePanelWidth: 0.3333,
ExpandFocusedSidePanel: false,
MainPanelSplitMode: "flexible",
Language: "auto",
TimeFormat: "02 Jan 06",
ShortTimeFormat: time.Kitchen,
Theme: ThemeConfig{
ActiveBorderColor: []string{"green", "bold"},
InactiveBorderColor: []string{"default"},
Expand Down
12 changes: 6 additions & 6 deletions pkg/gui/controllers/staging_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ func (self *StagingController) GetKeybindings(opts types.KeybindingsOpts) []*typ
},
{
Key: opts.GetKey(opts.Config.Universal.Remove),
Handler: self.ResetSelection,
Description: self.c.Tr.ResetSelection,
Handler: self.DiscardSelection,
Description: self.c.Tr.DiscardSelection,
},
{
Key: opts.GetKey(opts.Config.Main.EditSelectHunk),
Expand Down Expand Up @@ -166,13 +166,13 @@ func (self *StagingController) ToggleStaged() error {
return self.applySelectionAndRefresh(self.staged)
}

func (self *StagingController) ResetSelection() error {
func (self *StagingController) DiscardSelection() error {
reset := func() error { return self.applySelectionAndRefresh(true) }

if !self.staged && !self.c.UserConfig.Gui.SkipUnstageLineWarning {
if !self.staged && !self.c.UserConfig.Gui.SkipDiscardChangeWarning {
return self.c.Confirm(types.ConfirmOpts{
Title: self.c.Tr.UnstageLinesTitle,
Prompt: self.c.Tr.UnstageLinesPrompt,
Title: self.c.Tr.DiscardChangeTitle,
Prompt: self.c.Tr.DiscardChangePrompt,
HandleConfirm: reset,
})
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/i18n/chinese.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ func chineseTranslationSet() TranslationSet {
FileEnter: `暂存单个 块/行 用于文件, 或 折叠/展开 目录`,
FileStagingRequirements: `只能暂存跟踪文件的单独行`,
StageSelection: `切换行暂存状态`,
ResetSelection: `取消变更 (git reset)`,
DiscardSelection: `取消变更 (git reset)`,
ToggleDragSelect: `切换拖动选择`,
ToggleSelectHunk: `切换选择区块`,
ToggleSelectionForPatch: `添加/移除 行到补丁`,
Expand Down Expand Up @@ -378,8 +378,8 @@ func chineseTranslationSet() TranslationSet {
NoFilesStagedPrompt: "您尚未暂存任何文件。提交所有文件？",
BranchNotFoundTitle: "找不到分支",
BranchNotFoundPrompt: "找不到分支。创建一个新分支命名为：",
UnstageLinesTitle: "取消暂存选中的行",
UnstageLinesPrompt: "您确定要删除所选的行（git reset）吗？这是不可逆的。\n要禁用此对话框，请将 'gui.skipUnstageLineWarning' 的配置键设置为 true",
DiscardChangeTitle: "取消暂存选中的行",
DiscardChangePrompt: "您确定要删除所选的行（git reset）吗？这是不可逆的。\n要禁用此对话框，请将 'gui.skipDiscardChangeWarning' 的配置键设置为 true",
CreateNewBranchFromCommit: "从提交创建新分支",
BuildingPatch: "正在构建补丁",
ViewCommits: "查看提交",
Expand Down
2 changes: 1 addition & 1 deletion pkg/i18n/dutch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ func dutchTranslationSet() TranslationSet {
FileEnter: `Stage individuele hunks/lijnen`,
FileStagingRequirements: `Kan alleen individuele lijnen stagen van getrackte bestanden met onstaged veranderingen`,
StageSelection: `Toggle lijnen staged / unstaged`,
ResetSelection: `Verwijdert change (git reset)`,
DiscardSelection: `Verwijdert change (git reset)`,
ToggleDragSelect: `Toggle drag selecteer`,
ToggleSelectHunk: `Toggle selecteer hunk`,
ToggleSelectionForPatch: `Voeg toe/verwijder lijn(en) in patch`,
Expand Down
12 changes: 6 additions & 6 deletions pkg/i18n/english.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ type TranslationSet struct {
FileEnter string
FileStagingRequirements string
StageSelection string
ResetSelection string
DiscardSelection string
ToggleDragSelect string
ToggleSelectHunk string
ToggleSelectionForPatch string
Expand Down Expand Up @@ -428,8 +428,8 @@ type TranslationSet struct {
BranchNotFoundTitle string
BranchNotFoundPrompt string
BranchUnknown string
UnstageLinesTitle string
UnstageLinesPrompt string
DiscardChangeTitle string
DiscardChangePrompt string
CreateNewBranchFromCommit string
BuildingPatch string
ViewCommits string
Expand Down Expand Up @@ -863,7 +863,7 @@ func EnglishTranslationSet() TranslationSet {
FileEnter: `Stage individual hunks/lines for file, or collapse/expand for directory`,
FileStagingRequirements: `Can only stage individual lines for tracked files`,
StageSelection: `Toggle line staged / unstaged`,
ResetSelection: `Delete change (git reset)`,
DiscardSelection: `Discard change (git reset)`,
ToggleDragSelect: `Toggle drag select`,
ToggleSelectHunk: `Toggle select hunk`,
ToggleSelectionForPatch: `Add/Remove line(s) to patch`,
Expand Down Expand Up @@ -1119,8 +1119,8 @@ func EnglishTranslationSet() TranslationSet {
BranchNotFoundTitle: "Branch not found",
BranchNotFoundPrompt: "Branch not found. Create a new branch named",
BranchUnknown: "Branch unknown",
UnstageLinesTitle: "Unstage lines",
UnstageLinesPrompt: "Are you sure you want to delete the selected lines (git reset)? It is irreversible.\nTo disable this dialogue set the config key of 'gui.skipUnstageLineWarning' to true",
DiscardChangeTitle: "Discard change",
DiscardChangePrompt: "Are you sure you want to discard this change (git reset)? It is irreversible.\nTo disable this dialogue set the config key of 'gui.skipDiscardChangeWarning' to true",
CreateNewBranchFromCommit: "Create new branch off of commit",
BuildingPatch: "Building patch",
ViewCommits: "View commits",
Expand Down
6 changes: 3 additions & 3 deletions pkg/i18n/japanese.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ func japaneseTranslationSet() TranslationSet {
// FileEnter: `stage individual hunks/lines for file, or collapse/expand for directory`,
// FileStagingRequirements: `Can only stage individual lines for tracked files`,
StageSelection: `選択行をステージ/アンステージ`,
ResetSelection: `変更を削除 (git reset)`,
DiscardSelection: `変更を削除 (git reset)`,
ToggleDragSelect: `範囲選択を切り替え`,
ToggleSelectHunk: `Hunk選択を切り替え`,
ToggleSelectionForPatch: `行をパッチに追加/削除`,
Expand Down Expand Up @@ -394,8 +394,8 @@ func japaneseTranslationSet() TranslationSet {
NoFilesStagedPrompt: "ファイルがステージされていません。すべての変更をコミットしますか?",
BranchNotFoundTitle: "ブランチが見つかりませんでした。",
BranchNotFoundPrompt: "ブランチが見つかりませんでした。新しくブランチを作成します ",
UnstageLinesTitle: "選択行をアンステージ",
UnstageLinesPrompt: "選択された行を削除 (git reset) します。よろしいですか? この操作は取り消せません。\nこの警告を無効化するには設定ファイルの 'gui.skipUnstageLineWarning' を true に設定してください。",
DiscardChangeTitle: "選択行をアンステージ",
DiscardChangePrompt: "選択された行を削除 (git reset) します。よろしいですか? この操作は取り消せません。\nこの警告を無効化するには設定ファイルの 'gui.skipDiscardChangeWarning' を true に設定してください。",
CreateNewBranchFromCommit: "コミットにブランチを作成",
BuildingPatch: "パッチを構築",
ViewCommits: "コミットを閲覧",
Expand Down
6 changes: 3 additions & 3 deletions pkg/i18n/korean.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ func koreanTranslationSet() TranslationSet {
FileEnter: `Stage individual hunks/lines for file, or collapse/expand for directory`,
FileStagingRequirements: `추적된 파일에 대해 개별 라인만 stage할 수 있습니다.`,
StageSelection: `선택한 행을 staged / unstaged`,
ResetSelection: `변경을 삭제 (git reset)`,
DiscardSelection: `변경을 삭제 (git reset)`,
ToggleDragSelect: `드래그 선택 전환`,
ToggleSelectHunk: `Toggle select hunk`,
ToggleSelectionForPatch: `Line(s)을 패치에 추가/삭제`,
Expand Down Expand Up @@ -389,8 +389,8 @@ func koreanTranslationSet() TranslationSet {
NoFilesStagedPrompt: "파일이 Staged 되지 않았습니다. 모든 파일을 커밋하시겠습니까?",
BranchNotFoundTitle: "브랜치를 찾을 수 없습니다.",
BranchNotFoundPrompt: "브랜치를 찾을 수 없습니다. 새로운 브랜치를 생성합니다.",
UnstageLinesTitle: "선택한 라인을 unstaged",
UnstageLinesPrompt: "정말로 선택한 라인을 삭제 (git reset) 하시겠습니까? 이 조작은 취소할 수 없습니다.\n이 경고를 비활성화 하려면 설정 파일의 'gui.skipUnstageLineWarning' 를 true로 설정하세요.",
DiscardChangeTitle: "선택한 라인을 unstaged",
DiscardChangePrompt: "정말로 선택한 라인을 삭제 (git reset) 하시겠습니까? 이 조작은 취소할 수 없습니다.\n이 경고를 비활성화 하려면 설정 파일의 'gui.skipDiscardChangeWarning' 를 true로 설정하세요.",
CreateNewBranchFromCommit: "커밋에서 새 브랜치를 만듭니다.",
BuildingPatch: "Building patch",
ViewCommits: "커밋 보기",
Expand Down
6 changes: 3 additions & 3 deletions pkg/i18n/traditional_chinese.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ func traditionalChineseTranslationSet() TranslationSet {
FileEnter: `選擇檔案中的單個程式碼塊/行，或展開/折疊目錄`,
FileStagingRequirements: `只能選擇跟踪檔案中的單個行`,
StageSelection: `切換現有行的狀態 (已預存/未預存)`,
ResetSelection: `刪除變更 (git reset)`,
DiscardSelection: `刪除變更 (git reset)`,
ToggleDragSelect: `切換拖曳選擇`,
ToggleSelectHunk: `切換選擇程式碼塊`,
ToggleSelectionForPatch: `向 (或從) 補丁中添加/刪除行`,
Expand Down Expand Up @@ -481,8 +481,8 @@ func traditionalChineseTranslationSet() TranslationSet {
BranchNotFoundTitle: "找不到分支",
BranchNotFoundPrompt: "找不到分支。是否創建一個名為",
BranchUnknown: "分支未知",
UnstageLinesTitle: "取消預存行",
UnstageLinesPrompt: "你確定要刪除所選行嗎（git reset）？此操作是不可逆的。\n要禁用此對話框，請將“gui.skipUnstageLineWarning”設置為true。",
DiscardChangeTitle: "取消預存行",
DiscardChangePrompt: "你確定要刪除所選行嗎（git reset）？此操作是不可逆的。\n要禁用此對話框，請將“gui.skipDiscardChangeWarning”設置為true。",
CreateNewBranchFromCommit: "從提交建立新分支",
BuildingPatch: "正在建立補丁",
ViewCommits: "檢視提交",
Expand Down
4 changes: 2 additions & 2 deletions pkg/integration/components/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ func (self *Common) ContinueOnConflictsResolved() {

func (self *Common) ConfirmDiscardLines() {
self.t.ExpectPopup().Confirmation().
Title(Equals("Unstage lines")).
Content(Contains("Are you sure you want to delete the selected lines")).
Title(Equals("Discard change")).
Content(Contains("Are you sure you want to discard this change")).
Confirm()
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/integration/tests/staging/stage_lines.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ var StageLines = NewIntegrationTest(NewIntegrationTestArgs{
Press(keys.Universal.Remove).
Tap(func() {
t.ExpectPopup().Confirmation().
Title(Equals("Unstage lines")).
Content(Contains("Are you sure you want to delete the selected lines")).
Title(Equals("Discard change")).
Content(Contains("Are you sure you want to discard this change")).
Confirm()
}).
IsEmpty()
Expand Down