From 7f9818cfa2a3115ecd0f043b5e94044be1346cda Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 5 Sep 2023 21:49:33 +0200 Subject: [PATCH 1/7] Add DisabledReason field to MenuItem This is useful to disable items that are not applicable right now because of some condition (e.g. the "delete branch" menu item when the currently checked-out branch is selected). When a DisabledReason is set on a menu item, we - show it in a tooltip (below the regular tooltip of the item, if it has one) - strike through the item's key, if it has one - show an error message with the DisabledReason if the user tries to invoke the command --- pkg/gui/context/menu_context.go | 7 +++++++ pkg/gui/controllers/helpers/confirmation_helper.go | 13 ++++++++++++- pkg/gui/controllers/menu_controller.go | 2 +- pkg/gui/types/common.go | 4 ++++ pkg/i18n/english.go | 2 ++ 5 files changed, 26 insertions(+), 2 deletions(-) diff --git a/pkg/gui/context/menu_context.go b/pkg/gui/context/menu_context.go index 287ed92ec..f972f2fbb 100644 --- a/pkg/gui/context/menu_context.go +++ b/pkg/gui/context/menu_context.go @@ -90,6 +90,9 @@ func (self *MenuViewModel) GetDisplayStrings(_ int, _ int) [][]string { return lo.Map(menuItems, func(item *types.MenuItem, _ int) []string { displayStrings := item.LabelColumns + if item.DisabledReason != "" { + displayStrings[0] = style.FgDefault.SetStrikethrough().Sprint(displayStrings[0]) + } if !showKeys { return displayStrings @@ -169,6 +172,10 @@ func (self *MenuContext) GetKeybindings(opts types.KeybindingsOpts) []*types.Bin } func (self *MenuContext) OnMenuPress(selectedItem *types.MenuItem) error { + if selectedItem != nil && selectedItem.DisabledReason != "" { + return self.c.ErrorMsg(selectedItem.DisabledReason) + } + if err := self.c.PopContext(); err != nil { return err } diff --git a/pkg/gui/controllers/helpers/confirmation_helper.go b/pkg/gui/controllers/helpers/confirmation_helper.go index c920df39f..7d4ca1464 100644 --- a/pkg/gui/controllers/helpers/confirmation_helper.go +++ b/pkg/gui/controllers/helpers/confirmation_helper.go @@ -333,7 +333,7 @@ func (self *ConfirmationHelper) resizeMenu() { tooltip := "" selectedItem := self.c.Contexts().Menu.GetSelected() if selectedItem != nil { - tooltip = selectedItem.Tooltip + tooltip = self.TooltipForMenuItem(selectedItem) } tooltipHeight := getMessageHeight(true, tooltip, panelWidth) + 2 // plus 2 for the frame _, _ = self.c.GocuiGui().SetView(self.c.Views().Tooltip.Name(), x0, tooltipTop, x1, tooltipTop+tooltipHeight-1, 0) @@ -382,3 +382,14 @@ func (self *ConfirmationHelper) IsPopupPanel(viewName string) bool { func (self *ConfirmationHelper) IsPopupPanelFocused() bool { return self.IsPopupPanel(self.c.CurrentContext().GetViewName()) } + +func (self *ConfirmationHelper) TooltipForMenuItem(menuItem *types.MenuItem) string { + tooltip := menuItem.Tooltip + if menuItem.DisabledReason != "" { + if tooltip != "" { + tooltip += "\n\n" + } + tooltip += style.FgRed.Sprintf(self.c.Tr.DisabledMenuItemPrefix) + menuItem.DisabledReason + } + return tooltip +} diff --git a/pkg/gui/controllers/menu_controller.go b/pkg/gui/controllers/menu_controller.go index 108bd9cf7..0af32ef71 100644 --- a/pkg/gui/controllers/menu_controller.go +++ b/pkg/gui/controllers/menu_controller.go @@ -54,7 +54,7 @@ func (self *MenuController) GetOnFocus() func(types.OnFocusOpts) error { return func(types.OnFocusOpts) error { selectedMenuItem := self.context().GetSelected() if selectedMenuItem != nil { - self.c.Views().Tooltip.SetContent(selectedMenuItem.Tooltip) + self.c.Views().Tooltip.SetContent(self.c.Helpers().Confirmation.TooltipForMenuItem(selectedMenuItem)) } return nil } diff --git a/pkg/gui/types/common.go b/pkg/gui/types/common.go index 6eacd882b..21ba78d9a 100644 --- a/pkg/gui/types/common.go +++ b/pkg/gui/types/common.go @@ -204,6 +204,10 @@ type MenuItem struct { // The tooltip will be displayed upon highlighting the menu item Tooltip string + // If non-empty, show this in a tooltip, style the menu item as disabled, + // and refuse to invoke the command + DisabledReason string + // Can be used to group menu items into sections with headers. MenuItems // with the same Section should be contiguous, and will automatically get a // section header. If nil, the item is not part of a section. diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index 09577c210..0f788c317 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -612,6 +612,7 @@ type TranslationSet struct { MarkAsBaseCommitTooltip string MarkedCommitMarker string PleaseGoToURL string + DisabledMenuItemPrefix string Actions Actions Bisect Bisect Log Log @@ -1403,6 +1404,7 @@ func EnglishTranslationSet() TranslationSet { MarkAsBaseCommitTooltip: "Select a base commit for the next rebase; this will effectively perform a 'git rebase --onto'.", MarkedCommitMarker: "↑↑↑ Will rebase from here ↑↑↑", PleaseGoToURL: "Please go to {{.url}}", + DisabledMenuItemPrefix: "Disabled: ", Actions: Actions{ // TODO: combine this with the original keybinding descriptions (those are all in lowercase atm) CheckoutCommit: "Checkout commit", From 75aed98c3599afcb5791ec4a37319fe04db56736 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 5 Sep 2023 21:56:20 +0200 Subject: [PATCH 2/7] Use DisabledReason when deleting branches is not possible Two cases: trying to delete the currently checked-out branch, or deleting the upstream of a branch that doesn't have one. --- pkg/gui/controllers/branches_controller.go | 30 ++++++++-------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/pkg/gui/controllers/branches_controller.go b/pkg/gui/controllers/branches_controller.go index 1dc1f77e8..e60e5c410 100644 --- a/pkg/gui/controllers/branches_controller.go +++ b/pkg/gui/controllers/branches_controller.go @@ -468,7 +468,6 @@ func (self *BranchesController) forceDelete(branch *models.Branch) error { } func (self *BranchesController) delete(branch *models.Branch) error { - menuItems := []*types.MenuItem{} checkedOutBranch := self.c.Helpers().Refs.GetCheckedOutRef() localDeleteItem := &types.MenuItem{ @@ -479,25 +478,18 @@ func (self *BranchesController) delete(branch *models.Branch) error { }, } if checkedOutBranch.Name == branch.Name { - localDeleteItem = &types.MenuItem{ - Label: self.c.Tr.DeleteLocalBranch, - Key: 'c', - Tooltip: self.c.Tr.CantDeleteCheckOutBranch, - OnPress: func() error { - return self.c.ErrorMsg(self.c.Tr.CantDeleteCheckOutBranch) - }, - } + localDeleteItem.DisabledReason = self.c.Tr.CantDeleteCheckOutBranch } - menuItems = append(menuItems, localDeleteItem) - if branch.IsTrackingRemote() && !branch.UpstreamGone { - menuItems = append(menuItems, &types.MenuItem{ - Label: self.c.Tr.DeleteRemoteBranch, - Key: 'r', - OnPress: func() error { - return self.remoteDelete(branch) - }, - }) + remoteDeleteItem := &types.MenuItem{ + Label: self.c.Tr.DeleteRemoteBranch, + Key: 'r', + OnPress: func() error { + return self.remoteDelete(branch) + }, + } + if !branch.IsTrackingRemote() || branch.UpstreamGone { + remoteDeleteItem.DisabledReason = self.c.Tr.UpstreamNotSetError } menuTitle := utils.ResolvePlaceholderString( @@ -509,7 +501,7 @@ func (self *BranchesController) delete(branch *models.Branch) error { return self.c.Menu(types.CreateMenuOptions{ Title: menuTitle, - Items: menuItems, + Items: []*types.MenuItem{localDeleteItem, remoteDeleteItem}, }) } From f2f50ccf758033e817b20849a0175e7a383a3856 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 6 Sep 2023 09:40:28 +0200 Subject: [PATCH 3/7] Use DisabledReason for upstream options items --- pkg/gui/controllers/branches_controller.go | 173 +++++++++--------- pkg/i18n/english.go | 10 +- .../tests/branch/reset_to_upstream.go | 4 +- 3 files changed, 90 insertions(+), 97 deletions(-) diff --git a/pkg/gui/controllers/branches_controller.go b/pkg/gui/controllers/branches_controller.go index e60e5c410..bd7b37700 100644 --- a/pkg/gui/controllers/branches_controller.go +++ b/pkg/gui/controllers/branches_controller.go @@ -12,6 +12,7 @@ import ( "github.com/jesseduffield/lazygit/pkg/gui/controllers/helpers" "github.com/jesseduffield/lazygit/pkg/gui/types" "github.com/jesseduffield/lazygit/pkg/utils" + "github.com/samber/lo" ) type BranchesController struct { @@ -140,32 +141,55 @@ func (self *BranchesController) GetOnRenderToMain() func() error { } func (self *BranchesController) setUpstream(selectedBranch *models.Branch) error { - options := []*types.MenuItem{ - { - LabelColumns: []string{self.c.Tr.ViewDivergenceFromUpstream}, - OnPress: func() error { - branch := self.context().GetSelected() - if branch == nil { - return nil + viewDivergenceItem := &types.MenuItem{ + LabelColumns: []string{self.c.Tr.ViewDivergenceFromUpstream}, + OnPress: func() error { + branch := self.context().GetSelected() + if branch == nil { + return nil + } + + return self.c.Helpers().SubCommits.ViewSubCommits(helpers.ViewSubCommitsOpts{ + Ref: branch, + TitleRef: fmt.Sprintf("%s <-> %s", branch.RefName(), branch.ShortUpstreamRefName()), + RefToShowDivergenceFrom: branch.FullUpstreamRefName(), + Context: self.context(), + ShowBranchHeads: false, + }) + }, + Key: 'v', + } + + unsetUpstreamItem := &types.MenuItem{ + LabelColumns: []string{self.c.Tr.UnsetUpstream}, + OnPress: func() error { + if err := self.c.Git().Branch.UnsetUpstream(selectedBranch.Name); err != nil { + return self.c.Error(err) + } + if err := self.c.Refresh(types.RefreshOptions{ + Mode: types.SYNC, + Scope: []types.RefreshableView{ + types.BRANCHES, + types.COMMITS, + }, + }); err != nil { + return self.c.Error(err) + } + return nil + }, + Key: 'u', + } + + setUpstreamItem := &types.MenuItem{ + LabelColumns: []string{self.c.Tr.SetUpstream}, + OnPress: func() error { + return self.c.Helpers().Upstream.PromptForUpstreamWithoutInitialContent(selectedBranch, func(upstream string) error { + upstreamRemote, upstreamBranch, err := self.c.Helpers().Upstream.ParseUpstream(upstream) + if err != nil { + return self.c.Error(err) } - if !branch.RemoteBranchStoredLocally() { - return self.c.ErrorMsg(self.c.Tr.DivergenceNoUpstream) - } - return self.c.Helpers().SubCommits.ViewSubCommits(helpers.ViewSubCommitsOpts{ - Ref: branch, - TitleRef: fmt.Sprintf("%s <-> %s", branch.RefName(), branch.ShortUpstreamRefName()), - RefToShowDivergenceFrom: branch.FullUpstreamRefName(), - Context: self.context(), - ShowBranchHeads: false, - }) - }, - Key: 'v', - }, - { - LabelColumns: []string{self.c.Tr.UnsetUpstream}, - OnPress: func() error { - if err := self.c.Git().Branch.UnsetUpstream(selectedBranch.Name); err != nil { + if err := self.c.Git().Branch.SetUpstream(upstreamRemote, upstreamBranch, selectedBranch.Name); err != nil { return self.c.Error(err) } if err := self.c.Refresh(types.RefreshOptions{ @@ -178,75 +202,48 @@ func (self *BranchesController) setUpstream(selectedBranch *models.Branch) error return self.c.Error(err) } return nil - }, - Key: 'u', - }, - { - LabelColumns: []string{self.c.Tr.SetUpstream}, - OnPress: func() error { - return self.c.Helpers().Upstream.PromptForUpstreamWithoutInitialContent(selectedBranch, func(upstream string) error { - upstreamRemote, upstreamBranch, err := self.c.Helpers().Upstream.ParseUpstream(upstream) - if err != nil { - return self.c.Error(err) - } - - if err := self.c.Git().Branch.SetUpstream(upstreamRemote, upstreamBranch, selectedBranch.Name); err != nil { - return self.c.Error(err) - } - if err := self.c.Refresh(types.RefreshOptions{ - Mode: types.SYNC, - Scope: []types.RefreshableView{ - types.BRANCHES, - types.COMMITS, - }, - }); err != nil { - return self.c.Error(err) - } - return nil - }) - }, - Key: 's', + }) }, + Key: 's', } - if selectedBranch.IsTrackingRemote() { - upstream := fmt.Sprintf("%s/%s", selectedBranch.UpstreamRemote, selectedBranch.Name) - upstreamResetOptions := utils.ResolvePlaceholderString( - self.c.Tr.ViewUpstreamResetOptions, - map[string]string{"upstream": upstream}, - ) - upstreamResetTooltip := utils.ResolvePlaceholderString( - self.c.Tr.ViewUpstreamResetOptionsTooltip, - map[string]string{"upstream": upstream}, - ) + upstream := lo.Ternary(selectedBranch.RemoteBranchStoredLocally(), + fmt.Sprintf("%s/%s", selectedBranch.UpstreamRemote, selectedBranch.Name), + self.c.Tr.UpstreamGenericName) + upstreamResetOptions := utils.ResolvePlaceholderString( + self.c.Tr.ViewUpstreamResetOptions, + map[string]string{"upstream": upstream}, + ) + upstreamResetTooltip := utils.ResolvePlaceholderString( + self.c.Tr.ViewUpstreamResetOptionsTooltip, + map[string]string{"upstream": upstream}, + ) - options = append(options, &types.MenuItem{ - LabelColumns: []string{upstreamResetOptions}, - OpensMenu: true, - OnPress: func() error { - if selectedBranch.RemoteBranchNotStoredLocally() { - return self.c.ErrorMsg(self.c.Tr.UpstreamNotStoredLocallyError) - } + upstreamResetItem := &types.MenuItem{ + LabelColumns: []string{upstreamResetOptions}, + OpensMenu: true, + OnPress: func() error { + err := self.c.Helpers().Refs.CreateGitResetMenu(upstream) + if err != nil { + return self.c.Error(err) + } + return nil + }, + Tooltip: upstreamResetTooltip, + Key: 'g', + } - err := self.c.Helpers().Refs.CreateGitResetMenu(upstream) - if err != nil { - return self.c.Error(err) - } - return nil - }, - Tooltip: upstreamResetTooltip, - Key: 'g', - }) - } else { - options = append(options, &types.MenuItem{ - LabelColumns: []string{self.c.Tr.ViewUpstreamDisabledResetOptions}, - OpensMenu: true, - OnPress: func() error { - return self.c.ErrorMsg(self.c.Tr.UpstreamNotSetError) - }, - Tooltip: self.c.Tr.UpstreamNotSetError, - Key: 'g', - }) + if !selectedBranch.RemoteBranchStoredLocally() { + viewDivergenceItem.DisabledReason = self.c.Tr.UpstreamNotSetError + unsetUpstreamItem.DisabledReason = self.c.Tr.UpstreamNotSetError + upstreamResetItem.DisabledReason = self.c.Tr.UpstreamNotSetError + } + + options := []*types.MenuItem{ + viewDivergenceItem, + unsetUpstreamItem, + setUpstreamItem, + upstreamResetItem, } return self.c.Menu(types.CreateMenuOptions{ diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index 0f788c317..3182dfa68 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -352,12 +352,11 @@ type TranslationSet struct { SetUpstream string UnsetUpstream string ViewDivergenceFromUpstream string - DivergenceNoUpstream string DivergenceSectionHeaderLocal string DivergenceSectionHeaderRemote string ViewUpstreamResetOptions string ViewUpstreamResetOptionsTooltip string - ViewUpstreamDisabledResetOptions string + UpstreamGenericName string SetUpstreamTitle string SetUpstreamMessage string EditRemote string @@ -405,7 +404,6 @@ type TranslationSet struct { ViewBranchUpstreamOptions string BranchUpstreamOptionsTitle string ViewBranchUpstreamOptionsTooltip string - UpstreamNotStoredLocallyError string UpstreamNotSetError string NewGitFlowBranchPrompt string RenameBranchWarning string @@ -1148,12 +1146,11 @@ func EnglishTranslationSet() TranslationSet { SetUpstream: "Set upstream of selected branch", UnsetUpstream: "Unset upstream of selected branch", ViewDivergenceFromUpstream: "View divergence from upstream", - DivergenceNoUpstream: "Cannot show divergence of a branch that has no (locally tracked) upstream", DivergenceSectionHeaderLocal: "Local", DivergenceSectionHeaderRemote: "Remote", ViewUpstreamResetOptions: "Reset checked-out branch onto {{.upstream}}", ViewUpstreamResetOptionsTooltip: "View options for resetting the checked-out branch onto {{upstream}}. Note: this will not reset the selected branch onto the upstream, it will reset the checked-out branch onto the upstream", - ViewUpstreamDisabledResetOptions: "Reset checked-out branch onto upstream of selected branch", + UpstreamGenericName: "upstream of selected branch", SetUpstreamTitle: "Set upstream branch", SetUpstreamMessage: "Are you sure you want to set the upstream branch of '{{.checkedOut}}' to '{{.selected}}'", EditRemote: "Edit remote", @@ -1197,8 +1194,7 @@ func EnglishTranslationSet() TranslationSet { RenameBranch: "Rename branch", BranchUpstreamOptionsTitle: "Upstream options", ViewBranchUpstreamOptionsTooltip: "View options relating to the branch's upstream e.g. setting/unsetting the upstream and resetting to the upstream", - UpstreamNotStoredLocallyError: "Cannot reset to upstream branch because it is not stored locally", - UpstreamNotSetError: "The selected branch has no upstream", + UpstreamNotSetError: "The selected branch has no upstream (or the upstream is not stored locally)", ViewBranchUpstreamOptions: "View upstream options", NewBranchNamePrompt: "Enter new branch name for branch", RenameBranchWarning: "This branch is tracking a remote. This action will only rename the local branch name, not the name of the remote branch. Continue?", diff --git a/pkg/integration/tests/branch/reset_to_upstream.go b/pkg/integration/tests/branch/reset_to_upstream.go index 0c749ee2d..1eecd689b 100644 --- a/pkg/integration/tests/branch/reset_to_upstream.go +++ b/pkg/integration/tests/branch/reset_to_upstream.go @@ -41,11 +41,11 @@ var ResetToUpstream = NewIntegrationTest(NewIntegrationTestArgs{ t.ExpectPopup().Menu(). Title(Equals("Upstream options")). Select(Contains("Reset checked-out branch onto upstream of selected branch")). - Tooltip(Contains("The selected branch has no upstream")). + Tooltip(Contains("Disabled: The selected branch has no upstream (or the upstream is not stored locally)")). Confirm() t.ExpectPopup().Alert(). Title(Equals("Error")). - Content(Equals("The selected branch has no upstream")). + Content(Equals("The selected branch has no upstream (or the upstream is not stored locally)")). Confirm() }). SelectNextItem(). From e592d81b601d19d429ab61b7f3bd478de9844204 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 5 Sep 2023 22:27:28 +0200 Subject: [PATCH 4/7] Add Enabled func to Binding --- pkg/gui/controllers/options_menu_action.go | 13 +++++++++---- pkg/gui/gui_common.go | 4 ++++ pkg/gui/keybindings.go | 16 +++++++++++++++- pkg/gui/types/common.go | 1 + pkg/gui/types/keybindings.go | 7 +++++++ 5 files changed, 36 insertions(+), 5 deletions(-) diff --git a/pkg/gui/controllers/options_menu_action.go b/pkg/gui/controllers/options_menu_action.go index 341fe8fb3..27a2915b8 100644 --- a/pkg/gui/controllers/options_menu_action.go +++ b/pkg/gui/controllers/options_menu_action.go @@ -25,6 +25,10 @@ func (self *OptionsMenuAction) Call() error { appendBindings := func(bindings []*types.Binding, section *types.MenuSection) { menuItems = append(menuItems, lo.Map(bindings, func(binding *types.Binding, _ int) *types.MenuItem { + disabledReason := "" + if binding.GetDisabledReason != nil { + disabledReason = binding.GetDisabledReason() + } return &types.MenuItem{ OpensMenu: binding.OpensMenu, Label: binding.Description, @@ -33,11 +37,12 @@ func (self *OptionsMenuAction) Call() error { return nil } - return binding.Handler() + return self.c.IGuiCommon.CallKeybindingHandler(binding) }, - Key: binding.Key, - Tooltip: binding.Tooltip, - Section: section, + Key: binding.Key, + Tooltip: binding.Tooltip, + DisabledReason: disabledReason, + Section: section, } })...) } diff --git a/pkg/gui/gui_common.go b/pkg/gui/gui_common.go index bad0957ec..0abe94f4c 100644 --- a/pkg/gui/gui_common.go +++ b/pkg/gui/gui_common.go @@ -171,6 +171,10 @@ func (self *guiCommon) KeybindingsOpts() types.KeybindingsOpts { return self.gui.keybindingOpts() } +func (self *guiCommon) CallKeybindingHandler(binding *types.Binding) error { + return self.gui.callKeybindingHandler(binding) +} + func (self *guiCommon) IsAnyModeActive() bool { return self.gui.helpers.Mode.IsAnyModeActive() } diff --git a/pkg/gui/keybindings.go b/pkg/gui/keybindings.go index cb14d0266..afb12ce85 100644 --- a/pkg/gui/keybindings.go +++ b/pkg/gui/keybindings.go @@ -375,7 +375,10 @@ func (gui *Gui) wrappedHandler(f func() error) func(g *gocui.Gui, v *gocui.View) } func (gui *Gui) SetKeybinding(binding *types.Binding) error { - handler := binding.Handler + handler := func() error { + return gui.callKeybindingHandler(binding) + } + // TODO: move all mouse-ey stuff into new mouse approach if gocui.IsMouseKey(binding.Key) { handler = func() error { @@ -406,3 +409,14 @@ func (gui *Gui) SetMouseKeybinding(binding *gocui.ViewMouseBinding) error { return gui.g.SetViewClickBinding(binding) } + +func (gui *Gui) callKeybindingHandler(binding *types.Binding) error { + disabledReason := "" + if binding.GetDisabledReason != nil { + disabledReason = binding.GetDisabledReason() + } + if disabledReason != "" { + return gui.c.ErrorMsg(disabledReason) + } + return binding.Handler() +} diff --git a/pkg/gui/types/common.go b/pkg/gui/types/common.go index 21ba78d9a..5f1363f03 100644 --- a/pkg/gui/types/common.go +++ b/pkg/gui/types/common.go @@ -104,6 +104,7 @@ type IGuiCommon interface { State() IStateAccessor KeybindingsOpts() KeybindingsOpts + CallKeybindingHandler(binding *Binding) error // hopefully we can remove this once we've moved all our keybinding stuff out of the gui god struct. GetInitialKeybindingsWithCustomCommands() ([]*Binding, []*gocui.ViewMouseBinding) diff --git a/pkg/gui/types/keybindings.go b/pkg/gui/types/keybindings.go index 95978e762..4c3d02c6f 100644 --- a/pkg/gui/types/keybindings.go +++ b/pkg/gui/types/keybindings.go @@ -25,6 +25,13 @@ type Binding struct { // to be displayed if the keybinding is highlighted from within a menu Tooltip string + + // Function to decide whether the command is enabled, and why. If this + // returns an empty string, it is; if it returns a non-empty string, it is + // disabled and we show the given text in an error message when trying to + // invoke it. When left nil, the command is always enabled. Note that this + // function must not do expensive calls. + GetDisabledReason func() string } // A guard is a decorator which checks something before executing a handler From c9371f812fb584ee7d5c9ca9d7becccdfc5c9c3e Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 5 Sep 2023 22:45:56 +0200 Subject: [PATCH 5/7] Use DisabledReason for rebasing a branch onto itself --- pkg/gui/controllers/branches_controller.go | 17 ++++++++++++++--- .../helpers/merge_and_rebase_helper.go | 3 --- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/pkg/gui/controllers/branches_controller.go b/pkg/gui/controllers/branches_controller.go index bd7b37700..3c18b955c 100644 --- a/pkg/gui/controllers/branches_controller.go +++ b/pkg/gui/controllers/branches_controller.go @@ -76,9 +76,10 @@ func (self *BranchesController) GetKeybindings(opts types.KeybindingsOpts) []*ty OpensMenu: true, }, { - Key: opts.GetKey(opts.Config.Branches.RebaseBranch), - Handler: opts.Guards.OutsideFilterMode(self.rebase), - Description: self.c.Tr.RebaseBranch, + Key: opts.GetKey(opts.Config.Branches.RebaseBranch), + Handler: opts.Guards.OutsideFilterMode(self.rebase), + Description: self.c.Tr.RebaseBranch, + GetDisabledReason: self.getDisabledReasonForRebase, }, { Key: opts.GetKey(opts.Config.Branches.MergeIntoCurrentBranch), @@ -512,6 +513,16 @@ func (self *BranchesController) rebase() error { return self.c.Helpers().MergeAndRebase.RebaseOntoRef(selectedBranchName) } +func (self *BranchesController) getDisabledReasonForRebase() string { + selectedBranchName := self.context().GetSelected().Name + checkedOutBranch := self.c.Helpers().Refs.GetCheckedOutRef().Name + if selectedBranchName == checkedOutBranch { + return self.c.Tr.CantRebaseOntoSelf + } + + return "" +} + func (self *BranchesController) fastForward(branch *models.Branch) error { if !branch.IsTrackingRemote() { return self.c.ErrorMsg(self.c.Tr.FwdNoUpstream) diff --git a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go index a88615271..de34f2193 100644 --- a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go +++ b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go @@ -220,9 +220,6 @@ func (self *MergeAndRebaseHelper) PromptToContinueRebase() error { func (self *MergeAndRebaseHelper) RebaseOntoRef(ref string) error { checkedOutBranch := self.refsHelper.GetCheckedOutRef().Name - if ref == checkedOutBranch { - return self.c.ErrorMsg(self.c.Tr.CantRebaseOntoSelf) - } menuItems := []*types.MenuItem{ { Label: self.c.Tr.SimpleRebase, From 8b6766de79509617b02f4ab373f6d8ae3c0b4958 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 15 Sep 2023 10:30:48 +0200 Subject: [PATCH 6/7] Use DisabledReason for commits panel commands --- .../controllers/local_commits_controller.go | 232 ++++++++++++------ pkg/i18n/english.go | 2 + 2 files changed, 153 insertions(+), 81 deletions(-) diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 1bd079f53..a71968795 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -44,59 +44,70 @@ func NewLocalCommitsController( func (self *LocalCommitsController) GetKeybindings(opts types.KeybindingsOpts) []*types.Binding { outsideFilterModeBindings := []*types.Binding{ { - Key: opts.GetKey(opts.Config.Commits.SquashDown), - Handler: self.checkSelected(self.squashDown), - Description: self.c.Tr.SquashDown, + Key: opts.GetKey(opts.Config.Commits.SquashDown), + Handler: self.checkSelected(self.squashDown), + GetDisabledReason: self.callGetDisabledReasonFuncWithSelectedCommit(self.getDisabledReasonForSquashDown), + Description: self.c.Tr.SquashDown, }, { - Key: opts.GetKey(opts.Config.Commits.MarkCommitAsFixup), - Handler: self.checkSelected(self.fixup), - Description: self.c.Tr.FixupCommit, + Key: opts.GetKey(opts.Config.Commits.MarkCommitAsFixup), + Handler: self.checkSelected(self.fixup), + GetDisabledReason: self.callGetDisabledReasonFuncWithSelectedCommit(self.getDisabledReasonForFixup), + Description: self.c.Tr.FixupCommit, }, { - Key: opts.GetKey(opts.Config.Commits.RenameCommit), - Handler: self.checkSelected(self.reword), - Description: self.c.Tr.RewordCommit, + Key: opts.GetKey(opts.Config.Commits.RenameCommit), + Handler: self.checkSelected(self.reword), + GetDisabledReason: self.getDisabledReasonForRebaseCommandWithSelectedCommit(todo.Reword), + Description: self.c.Tr.RewordCommit, }, { - Key: opts.GetKey(opts.Config.Commits.RenameCommitWithEditor), - Handler: self.checkSelected(self.rewordEditor), - Description: self.c.Tr.RenameCommitEditor, + Key: opts.GetKey(opts.Config.Commits.RenameCommitWithEditor), + Handler: self.checkSelected(self.rewordEditor), + GetDisabledReason: self.getDisabledReasonForRebaseCommandWithSelectedCommit(todo.Reword), + Description: self.c.Tr.RenameCommitEditor, }, { - Key: opts.GetKey(opts.Config.Universal.Remove), - Handler: self.checkSelected(self.drop), - Description: self.c.Tr.DeleteCommit, + Key: opts.GetKey(opts.Config.Universal.Remove), + Handler: self.checkSelected(self.drop), + GetDisabledReason: self.getDisabledReasonForRebaseCommandWithSelectedCommit(todo.Drop), + Description: self.c.Tr.DeleteCommit, }, { - Key: opts.GetKey(opts.Config.Universal.Edit), - Handler: self.checkSelected(self.edit), - Description: self.c.Tr.EditCommit, + Key: opts.GetKey(opts.Config.Universal.Edit), + Handler: self.checkSelected(self.edit), + GetDisabledReason: self.getDisabledReasonForRebaseCommandWithSelectedCommit(todo.Edit), + Description: self.c.Tr.EditCommit, }, { - Key: opts.GetKey(opts.Config.Commits.PickCommit), - Handler: self.checkSelected(self.pick), - Description: self.c.Tr.PickCommit, + Key: opts.GetKey(opts.Config.Commits.PickCommit), + Handler: self.checkSelected(self.pick), + GetDisabledReason: self.getDisabledReasonForRebaseCommandWithSelectedCommit(todo.Pick), + Description: self.c.Tr.PickCommit, }, { - Key: opts.GetKey(opts.Config.Commits.CreateFixupCommit), - Handler: self.checkSelected(self.createFixupCommit), - Description: self.c.Tr.CreateFixupCommitDescription, + Key: opts.GetKey(opts.Config.Commits.CreateFixupCommit), + Handler: self.checkSelected(self.createFixupCommit), + GetDisabledReason: self.disabledIfNoSelectedCommit(), + Description: self.c.Tr.CreateFixupCommitDescription, }, { - Key: opts.GetKey(opts.Config.Commits.SquashAboveCommits), - Handler: self.checkSelected(self.squashAllAboveFixupCommits), - Description: self.c.Tr.SquashAboveCommits, + Key: opts.GetKey(opts.Config.Commits.SquashAboveCommits), + Handler: self.checkSelected(self.squashAllAboveFixupCommits), + GetDisabledReason: self.callGetDisabledReasonFuncWithSelectedCommit(self.getDisabledReasonForSquashAllAboveFixupCommits), + Description: self.c.Tr.SquashAboveCommits, }, { - Key: opts.GetKey(opts.Config.Commits.MoveDownCommit), - Handler: self.checkSelected(self.moveDown), - Description: self.c.Tr.MoveDownCommit, + Key: opts.GetKey(opts.Config.Commits.MoveDownCommit), + Handler: self.checkSelected(self.moveDown), + GetDisabledReason: self.disabledIfNoSelectedCommit(), + Description: self.c.Tr.MoveDownCommit, }, { - Key: opts.GetKey(opts.Config.Commits.MoveUpCommit), - Handler: self.checkSelected(self.moveUp), - Description: self.c.Tr.MoveUpCommit, + Key: opts.GetKey(opts.Config.Commits.MoveUpCommit), + Handler: self.checkSelected(self.moveUp), + GetDisabledReason: self.disabledIfNoSelectedCommit(), + Description: self.c.Tr.MoveUpCommit, }, { Key: opts.GetKey(opts.Config.Commits.PasteCommits), @@ -104,10 +115,11 @@ func (self *LocalCommitsController) GetKeybindings(opts types.KeybindingsOpts) [ Description: self.c.Tr.PasteCommits, }, { - Key: opts.GetKey(opts.Config.Commits.MarkCommitAsBaseForRebase), - Handler: self.checkSelected(self.markAsBaseCommit), - Description: self.c.Tr.MarkAsBaseCommit, - Tooltip: self.c.Tr.MarkAsBaseCommitTooltip, + Key: opts.GetKey(opts.Config.Commits.MarkCommitAsBaseForRebase), + Handler: self.checkSelected(self.markAsBaseCommit), + GetDisabledReason: self.disabledIfNoSelectedCommit(), + Description: self.c.Tr.MarkAsBaseCommit, + Tooltip: self.c.Tr.MarkAsBaseCommitTooltip, }, // overriding these navigation keybindings because we might need to load // more commits on demand @@ -131,25 +143,29 @@ func (self *LocalCommitsController) GetKeybindings(opts types.KeybindingsOpts) [ bindings := append(outsideFilterModeBindings, []*types.Binding{ { - Key: opts.GetKey(opts.Config.Commits.AmendToCommit), - Handler: self.checkSelected(self.amendTo), - Description: self.c.Tr.AmendToCommit, + Key: opts.GetKey(opts.Config.Commits.AmendToCommit), + Handler: self.checkSelected(self.amendTo), + GetDisabledReason: self.callGetDisabledReasonFuncWithSelectedCommit(self.getDisabledReasonForAmendTo), + Description: self.c.Tr.AmendToCommit, }, { - Key: opts.GetKey(opts.Config.Commits.ResetCommitAuthor), - Handler: self.checkSelected(self.amendAttribute), - Description: self.c.Tr.SetResetCommitAuthor, - OpensMenu: true, + Key: opts.GetKey(opts.Config.Commits.ResetCommitAuthor), + Handler: self.checkSelected(self.amendAttribute), + GetDisabledReason: self.callGetDisabledReasonFuncWithSelectedCommit(self.getDisabledReasonForAmendTo), + Description: self.c.Tr.SetResetCommitAuthor, + OpensMenu: true, }, { - Key: opts.GetKey(opts.Config.Commits.RevertCommit), - Handler: self.checkSelected(self.revert), - Description: self.c.Tr.RevertCommit, + Key: opts.GetKey(opts.Config.Commits.RevertCommit), + Handler: self.checkSelected(self.revert), + GetDisabledReason: self.disabledIfNoSelectedCommit(), + Description: self.c.Tr.RevertCommit, }, { - Key: opts.GetKey(opts.Config.Commits.CreateTag), - Handler: self.checkSelected(self.createTag), - Description: self.c.Tr.TagCommit, + Key: opts.GetKey(opts.Config.Commits.CreateTag), + Handler: self.checkSelected(self.createTag), + GetDisabledReason: self.disabledIfNoSelectedCommit(), + Description: self.c.Tr.TagCommit, }, { Key: opts.GetKey(opts.Config.Commits.OpenLogMenu), @@ -208,10 +224,6 @@ func secondaryPatchPanelUpdateOpts(c *ControllerCommon) *types.ViewUpdateOpts { } func (self *LocalCommitsController) squashDown(commit *models.Commit) error { - if self.context().GetSelectedLineIdx() >= len(self.c.Model().Commits)-1 { - return self.c.ErrorMsg(self.c.Tr.CannotSquashOrFixupFirstCommit) - } - applied, err := self.handleMidRebaseCommand(todo.Squash, commit) if err != nil { return err @@ -232,11 +244,15 @@ func (self *LocalCommitsController) squashDown(commit *models.Commit) error { }) } -func (self *LocalCommitsController) fixup(commit *models.Commit) error { +func (self *LocalCommitsController) getDisabledReasonForSquashDown(commit *models.Commit) string { if self.context().GetSelectedLineIdx() >= len(self.c.Model().Commits)-1 { - return self.c.ErrorMsg(self.c.Tr.CannotSquashOrFixupFirstCommit) + return self.c.Tr.CannotSquashOrFixupFirstCommit } + return self.rebaseCommandEnabled(todo.Squash, commit) +} + +func (self *LocalCommitsController) fixup(commit *models.Commit) error { applied, err := self.handleMidRebaseCommand(todo.Fixup, commit) if err != nil { return err @@ -257,6 +273,14 @@ func (self *LocalCommitsController) fixup(commit *models.Commit) error { }) } +func (self *LocalCommitsController) getDisabledReasonForFixup(commit *models.Commit) string { + if self.context().GetSelectedLineIdx() >= len(self.c.Model().Commits)-1 { + return self.c.Tr.CannotSquashOrFixupFirstCommit + } + + return self.rebaseCommandEnabled(todo.Squash, commit) +} + func (self *LocalCommitsController) reword(commit *models.Commit) error { applied, err := self.handleMidRebaseCommand(todo.Reword, commit) if err != nil { @@ -428,34 +452,10 @@ func (self *LocalCommitsController) interactiveRebase(action todo.TodoCommand) e // commit meaning you are trying to edit the todo file rather than actually // begin a rebase. It then updates the todo file with that action func (self *LocalCommitsController) handleMidRebaseCommand(action todo.TodoCommand, commit *models.Commit) (bool, error) { - if commit.Action == models.ActionConflict { - return true, self.c.ErrorMsg(self.c.Tr.ChangingThisActionIsNotAllowed) - } - if !commit.IsTODO() { - if self.c.Git().Status.WorkingTreeState() != enums.REBASE_MODE_NONE { - // If we are in a rebase, the only action that is allowed for - // non-todo commits is rewording the current head commit - if !(action == todo.Reword && self.isHeadCommit()) { - return true, self.c.ErrorMsg(self.c.Tr.AlreadyRebasing) - } - } - return false, nil } - // for now we do not support setting 'reword' because it requires an editor - // and that means we either unconditionally wait around for the subprocess to ask for - // our input or we set a lazygit client as the EDITOR env variable and have it - // request us to edit the commit message when prompted. - if action == todo.Reword { - return true, self.c.ErrorMsg(self.c.Tr.RewordNotSupported) - } - - if allowed := isChangeOfRebaseTodoAllowed(action); !allowed { - return true, self.c.ErrorMsg(self.c.Tr.ChangingThisActionIsNotAllowed) - } - self.c.LogAction("Update rebase TODO") msg := utils.ResolvePlaceholderString( @@ -476,6 +476,38 @@ func (self *LocalCommitsController) handleMidRebaseCommand(action todo.TodoComma }) } +func (self *LocalCommitsController) rebaseCommandEnabled(action todo.TodoCommand, commit *models.Commit) string { + if commit.Action == models.ActionConflict { + return self.c.Tr.ChangingThisActionIsNotAllowed + } + + if !commit.IsTODO() { + if self.c.Git().Status.WorkingTreeState() != enums.REBASE_MODE_NONE { + // If we are in a rebase, the only action that is allowed for + // non-todo commits is rewording the current head commit + if !(action == todo.Reword && self.isHeadCommit()) { + return self.c.Tr.AlreadyRebasing + } + } + + return "" + } + + // for now we do not support setting 'reword' because it requires an editor + // and that means we either unconditionally wait around for the subprocess to ask for + // our input or we set a lazygit client as the EDITOR env variable and have it + // request us to edit the commit message when prompted. + if action == todo.Reword { + return self.c.Tr.RewordNotSupported + } + + if allowed := isChangeOfRebaseTodoAllowed(action); !allowed { + return self.c.Tr.ChangingThisActionIsNotAllowed + } + + return "" +} + func (self *LocalCommitsController) moveDown(commit *models.Commit) error { index := self.context().GetSelectedLineIdx() commits := self.c.Model().Commits @@ -601,6 +633,14 @@ func (self *LocalCommitsController) amendTo(commit *models.Commit) error { }) } +func (self *LocalCommitsController) getDisabledReasonForAmendTo(commit *models.Commit) string { + if !self.isHeadCommit() && self.c.Git().Status.WorkingTreeState() != enums.REBASE_MODE_NONE { + return self.c.Tr.AlreadyRebasing + } + + return "" +} + func (self *LocalCommitsController) amendAttribute(commit *models.Commit) error { if self.c.Git().Status.WorkingTreeState() != enums.REBASE_MODE_NONE && !self.isHeadCommit() { return self.c.ErrorMsg(self.c.Tr.AlreadyRebasing) @@ -772,6 +812,14 @@ func (self *LocalCommitsController) squashAllAboveFixupCommits(commit *models.Co }) } +func (self *LocalCommitsController) getDisabledReasonForSquashAllAboveFixupCommits(commit *models.Commit) string { + if self.c.Git().Status.WorkingTreeState() != enums.REBASE_MODE_NONE { + return self.c.Tr.AlreadyRebasing + } + + return "" +} + func (self *LocalCommitsController) createTag(commit *models.Commit) error { return self.c.Helpers().Tags.OpenCreateTagPrompt(commit.Sha, func() {}) } @@ -896,13 +944,35 @@ func (self *LocalCommitsController) checkSelected(callback func(*models.Commit) return func() error { commit := self.context().GetSelected() if commit == nil { - return nil + // The enabled callback should have checked for this + panic("no commit selected") } return callback(commit) } } +func (self *LocalCommitsController) callGetDisabledReasonFuncWithSelectedCommit(callback func(*models.Commit) string) func() string { + return func() string { + commit := self.context().GetSelected() + if commit == nil { + return self.c.Tr.NoCommitSelected + } + + return callback(commit) + } +} + +func (self *LocalCommitsController) disabledIfNoSelectedCommit() func() string { + return self.callGetDisabledReasonFuncWithSelectedCommit(func(*models.Commit) string { return "" }) +} + +func (self *LocalCommitsController) getDisabledReasonForRebaseCommandWithSelectedCommit(action todo.TodoCommand) func() string { + return self.callGetDisabledReasonFuncWithSelectedCommit(func(commit *models.Commit) string { + return self.rebaseCommandEnabled(action, commit) + }) +} + func (self *LocalCommitsController) GetOnFocus() func(types.OnFocusOpts) error { return func(types.OnFocusOpts) error { context := self.context() diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index 3182dfa68..0f30f72f7 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -611,6 +611,7 @@ type TranslationSet struct { MarkedCommitMarker string PleaseGoToURL string DisabledMenuItemPrefix string + NoCommitSelected string Actions Actions Bisect Bisect Log Log @@ -1401,6 +1402,7 @@ func EnglishTranslationSet() TranslationSet { MarkedCommitMarker: "↑↑↑ Will rebase from here ↑↑↑", PleaseGoToURL: "Please go to {{.url}}", DisabledMenuItemPrefix: "Disabled: ", + NoCommitSelected: "No commit selected", Actions: Actions{ // TODO: combine this with the original keybinding descriptions (those are all in lowercase atm) CheckoutCommit: "Checkout commit", From 0b13c3ca87313a94d5c7972db39b7e64fbea5d67 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 15 Sep 2023 17:46:21 +0200 Subject: [PATCH 7/7] Disabled paste when there are no copied commits --- pkg/gui/controllers/helpers/cherry_pick_helper.go | 4 ++++ pkg/gui/controllers/local_commits_controller.go | 15 ++++++++++++--- pkg/i18n/english.go | 2 ++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/pkg/gui/controllers/helpers/cherry_pick_helper.go b/pkg/gui/controllers/helpers/cherry_pick_helper.go index 2e8a11f7d..e27e469b6 100644 --- a/pkg/gui/controllers/helpers/cherry_pick_helper.go +++ b/pkg/gui/controllers/helpers/cherry_pick_helper.go @@ -85,6 +85,10 @@ func (self *CherryPickHelper) Paste() error { }) } +func (self *CherryPickHelper) CanPaste() bool { + return self.getData().Active() +} + func (self *CherryPickHelper) Reset() error { self.getData().ContextKey = "" self.getData().CherryPickedCommits = nil diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index a71968795..2c4794195 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -110,9 +110,10 @@ func (self *LocalCommitsController) GetKeybindings(opts types.KeybindingsOpts) [ Description: self.c.Tr.MoveUpCommit, }, { - Key: opts.GetKey(opts.Config.Commits.PasteCommits), - Handler: self.paste, - Description: self.c.Tr.PasteCommits, + Key: opts.GetKey(opts.Config.Commits.PasteCommits), + Handler: self.paste, + GetDisabledReason: self.getDisabledReasonForPaste, + Description: self.c.Tr.PasteCommits, }, { Key: opts.GetKey(opts.Config.Commits.MarkCommitAsBaseForRebase), @@ -1001,6 +1002,14 @@ func (self *LocalCommitsController) paste() error { return self.c.Helpers().CherryPick.Paste() } +func (self *LocalCommitsController) getDisabledReasonForPaste() string { + if !self.c.Helpers().CherryPick.CanPaste() { + return self.c.Tr.NoCopiedCommits + } + + return "" +} + func (self *LocalCommitsController) markAsBaseCommit(commit *models.Commit) error { if commit.Sha == self.c.Modes().MarkedBaseCommit.GetSha() { // Reset when invoking it again on the marked commit diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index 0f30f72f7..0aa1baeb8 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -612,6 +612,7 @@ type TranslationSet struct { PleaseGoToURL string DisabledMenuItemPrefix string NoCommitSelected string + NoCopiedCommits string Actions Actions Bisect Bisect Log Log @@ -1403,6 +1404,7 @@ func EnglishTranslationSet() TranslationSet { PleaseGoToURL: "Please go to {{.url}}", DisabledMenuItemPrefix: "Disabled: ", NoCommitSelected: "No commit selected", + NoCopiedCommits: "No copied commits", Actions: Actions{ // TODO: combine this with the original keybinding descriptions (those are all in lowercase atm) CheckoutCommit: "Checkout commit",