From c59e6b6451da5eaa9ce4c7e4d581fb44d17ca53a Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 27 Mar 2024 19:22:24 +0100 Subject: [PATCH 1/5] Cleanup: don't mess with globals in tests without resetting them Changing globals in the init() function of a test file is a bad idea, as it affects all other tests that run after it. Do it explicitly in each test function that needs it, and take care of restoring the previous value afterwards. --- pkg/gui/presentation/branches_test.go | 5 +++++ pkg/gui/presentation/commits_test.go | 7 +++---- pkg/gui/presentation/files_test.go | 10 ++++++---- pkg/gui/presentation/graph/graph_test.go | 17 ++++++++++++----- pkg/gui/style/style_test.go | 11 ++++++----- 5 files changed, 32 insertions(+), 18 deletions(-) diff --git a/pkg/gui/presentation/branches_test.go b/pkg/gui/presentation/branches_test.go index 2414572ca..250b143e3 100644 --- a/pkg/gui/presentation/branches_test.go +++ b/pkg/gui/presentation/branches_test.go @@ -5,12 +5,14 @@ import ( "testing" "time" + "github.com/gookit/color" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/gui/presentation/icons" "github.com/jesseduffield/lazygit/pkg/gui/types" "github.com/jesseduffield/lazygit/pkg/utils" "github.com/samber/lo" "github.com/stretchr/testify/assert" + "github.com/xo/terminfo" ) func Test_getBranchDisplayStrings(t *testing.T) { @@ -223,6 +225,9 @@ func Test_getBranchDisplayStrings(t *testing.T) { }, } + oldColorLevel := color.ForceSetColorLevel(terminfo.ColorLevelNone) + defer color.ForceSetColorLevel(oldColorLevel) + c := utils.NewDummyCommon() for i, s := range scenarios { diff --git a/pkg/gui/presentation/commits_test.go b/pkg/gui/presentation/commits_test.go index 16f1de660..f1f075f45 100644 --- a/pkg/gui/presentation/commits_test.go +++ b/pkg/gui/presentation/commits_test.go @@ -16,10 +16,6 @@ import ( "github.com/xo/terminfo" ) -func init() { - color.ForceSetColorLevel(terminfo.ColorLevelNone) -} - func formatExpected(expected string) string { return strings.TrimSpace(strings.ReplaceAll(expected, "\t", "")) } @@ -385,6 +381,9 @@ func TestGetCommitListDisplayStrings(t *testing.T) { }, } + oldColorLevel := color.ForceSetColorLevel(terminfo.ColorLevelNone) + defer color.ForceSetColorLevel(oldColorLevel) + os.Setenv("TZ", "UTC") focusing := false diff --git a/pkg/gui/presentation/files_test.go b/pkg/gui/presentation/files_test.go index bbaa53947..6662c7732 100644 --- a/pkg/gui/presentation/files_test.go +++ b/pkg/gui/presentation/files_test.go @@ -13,10 +13,6 @@ import ( "github.com/xo/terminfo" ) -func init() { - color.ForceSetColorLevel(terminfo.ColorLevelNone) -} - func toStringSlice(str string) []string { return strings.Split(strings.TrimSpace(str), "\n") } @@ -66,6 +62,9 @@ M file1 }, } + oldColorLevel := color.ForceSetColorLevel(terminfo.ColorLevelNone) + defer color.ForceSetColorLevel(oldColorLevel) + for _, s := range scenarios { s := s t.Run(s.name, func(t *testing.T) { @@ -125,6 +124,9 @@ M file1 }, } + oldColorLevel := color.ForceSetColorLevel(terminfo.ColorLevelNone) + defer color.ForceSetColorLevel(oldColorLevel) + for _, s := range scenarios { s := s t.Run(s.name, func(t *testing.T) { diff --git a/pkg/gui/presentation/graph/graph_test.go b/pkg/gui/presentation/graph/graph_test.go index 834575d7b..a7fe5879b 100644 --- a/pkg/gui/presentation/graph/graph_test.go +++ b/pkg/gui/presentation/graph/graph_test.go @@ -15,11 +15,6 @@ import ( "github.com/xo/terminfo" ) -func init() { - // on CI we've got no color capability so we're forcing it here - color.ForceSetColorLevel(terminfo.ColorLevelMillions) -} - func TestRenderCommitGraph(t *testing.T) { tests := []struct { name string @@ -218,6 +213,9 @@ func TestRenderCommitGraph(t *testing.T) { }, } + oldColorLevel := color.ForceSetColorLevel(terminfo.ColorLevelMillions) + defer color.ForceSetColorLevel(oldColorLevel) + for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { @@ -452,6 +450,9 @@ func TestRenderPipeSet(t *testing.T) { }, } + oldColorLevel := color.ForceSetColorLevel(terminfo.ColorLevelMillions) + defer color.ForceSetColorLevel(oldColorLevel) + for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { @@ -523,6 +524,9 @@ func TestGetNextPipes(t *testing.T) { }, } + oldColorLevel := color.ForceSetColorLevel(terminfo.ColorLevelMillions) + defer color.ForceSetColorLevel(oldColorLevel) + for _, test := range tests { getStyle := func(c *models.Commit) style.TextStyle { return style.FgDefault } pipes := getNextPipes(test.prevPipes, test.commit, getStyle) @@ -538,6 +542,9 @@ func TestGetNextPipes(t *testing.T) { } func BenchmarkRenderCommitGraph(b *testing.B) { + oldColorLevel := color.ForceSetColorLevel(terminfo.ColorLevelMillions) + defer color.ForceSetColorLevel(oldColorLevel) + commits := generateCommits(50) getStyle := func(commit *models.Commit) style.TextStyle { return authors.AuthorStyle(commit.AuthorName) diff --git a/pkg/gui/style/style_test.go b/pkg/gui/style/style_test.go index c8157efd6..12fec4287 100644 --- a/pkg/gui/style/style_test.go +++ b/pkg/gui/style/style_test.go @@ -10,11 +10,6 @@ import ( "github.com/xo/terminfo" ) -func init() { - // on CI we've got no color capability so we're forcing it here - color.ForceSetColorLevel(terminfo.ColorLevelMillions) -} - func TestMerge(t *testing.T) { type scenario struct { name string @@ -162,6 +157,9 @@ func TestMerge(t *testing.T) { }, } + oldColorLevel := color.ForceSetColorLevel(terminfo.ColorLevelMillions) + defer color.ForceSetColorLevel(oldColorLevel) + for _, s := range scenarios { s := s t.Run(s.name, func(t *testing.T) { @@ -210,6 +208,9 @@ func TestTemplateFuncMapAddColors(t *testing.T) { }, } + oldColorLevel := color.ForceSetColorLevel(terminfo.ColorLevelMillions) + defer color.ForceSetColorLevel(oldColorLevel) + for _, s := range scenarios { s := s t.Run(s.name, func(t *testing.T) { From 6396d1ce0358dc583d7844797f07431ab5452447 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 27 Mar 2024 18:37:30 +0100 Subject: [PATCH 2/5] Extract a function HandleGenericClick --- pkg/gui/controllers/status_controller.go | 13 +------------ pkg/gui/gui_common.go | 4 ++++ pkg/gui/types/common.go | 4 ++++ pkg/gui/view_helpers.go | 16 ++++++++++++++++ 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/pkg/gui/controllers/status_controller.go b/pkg/gui/controllers/status_controller.go index e8455fe22..49a182fba 100644 --- a/pkg/gui/controllers/status_controller.go +++ b/pkg/gui/controllers/status_controller.go @@ -79,18 +79,7 @@ func (self *StatusController) GetMouseKeybindings(opts types.KeybindingsOpts) [] } func (self *StatusController) onClickMain(opts gocui.ViewMouseBindingOpts) error { - view := self.c.Views().Main - - cx, cy := view.Cursor() - url, err := view.Word(cx, cy) - if err == nil && strings.HasPrefix(url, "https://") { - // Ignore errors (opening the link via the OS can fail if the - // `os.openLink` config key references a command that doesn't exist, or - // that errors when called.) - _ = self.c.OS().OpenLink(url) - } - - return nil + return self.c.HandleGenericClick(self.c.Views().Main) } func (self *StatusController) GetOnRenderToMain() func() error { diff --git a/pkg/gui/gui_common.go b/pkg/gui/gui_common.go index 4b9d3dc37..f45440312 100644 --- a/pkg/gui/gui_common.go +++ b/pkg/gui/gui_common.go @@ -33,6 +33,10 @@ func (self *guiCommon) PostRefreshUpdate(context types.Context) error { return self.gui.postRefreshUpdate(context) } +func (self *guiCommon) HandleGenericClick(view *gocui.View) error { + return self.gui.handleGenericClick(view) +} + func (self *guiCommon) RunSubprocessAndRefresh(cmdObj oscommands.ICmdObj) error { return self.gui.runSubprocessWithSuspenseAndRefresh(cmdObj) } diff --git a/pkg/gui/types/common.go b/pkg/gui/types/common.go index e53260b34..694cdcc56 100644 --- a/pkg/gui/types/common.go +++ b/pkg/gui/types/common.go @@ -35,6 +35,10 @@ type IGuiCommon interface { // case would be overkill, although refresh will internally call 'PostRefreshUpdate' PostRefreshUpdate(Context) error + // a generic click handler that can be used for any view; it handles opening + // URLs in the browser when the user clicks on one + HandleGenericClick(view *gocui.View) error + // renders string to a view without resetting its origin SetViewContent(view *gocui.View, content string) // resets cursor and origin of view. Often used before calling SetViewContent diff --git a/pkg/gui/view_helpers.go b/pkg/gui/view_helpers.go index 22126cc33..3f5f27f55 100644 --- a/pkg/gui/view_helpers.go +++ b/pkg/gui/view_helpers.go @@ -1,6 +1,7 @@ package gui import ( + "strings" "time" "github.com/jesseduffield/gocui" @@ -148,3 +149,18 @@ func (gui *Gui) postRefreshUpdate(c types.Context) error { return nil } + +// handleGenericClick is a generic click handler that can be used for any view. +// It handles opening URLs in the browser when the user clicks on one. +func (gui *Gui) handleGenericClick(view *gocui.View) error { + cx, cy := view.Cursor() + url, err := view.Word(cx, cy) + if err == nil && strings.HasPrefix(url, "https://") { + // Ignore errors (opening the link via the OS can fail if the + // `os.openLink` config key references a command that doesn't exist, or + // that errors when called.) + _ = gui.c.OS().OpenLink(url) + } + + return nil +} From d102f12304177a5bdb5dd3d21eea7a31f59bfc16 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 27 Mar 2024 18:44:51 +0100 Subject: [PATCH 3/5] Make HandleGenericClick a little smarter Make it recognize URLs wrapped in angle brackets, and followed by punktuation. We don't need this for the status panel, but we will need it for confirmation panels. --- pkg/gui/view_helpers.go | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/pkg/gui/view_helpers.go b/pkg/gui/view_helpers.go index 3f5f27f55..1043920ec 100644 --- a/pkg/gui/view_helpers.go +++ b/pkg/gui/view_helpers.go @@ -1,7 +1,7 @@ package gui import ( - "strings" + "regexp" "time" "github.com/jesseduffield/gocui" @@ -154,13 +154,23 @@ func (gui *Gui) postRefreshUpdate(c types.Context) error { // It handles opening URLs in the browser when the user clicks on one. func (gui *Gui) handleGenericClick(view *gocui.View) error { cx, cy := view.Cursor() - url, err := view.Word(cx, cy) - if err == nil && strings.HasPrefix(url, "https://") { - // Ignore errors (opening the link via the OS can fail if the - // `os.openLink` config key references a command that doesn't exist, or - // that errors when called.) - _ = gui.c.OS().OpenLink(url) + word, err := view.Word(cx, cy) + if err != nil { + return nil } + // Allow URLs to be wrapped in angle brackets, and the closing bracket to + // be followed by punctuation: + re := regexp.MustCompile(`^[,.;!]*)?$`) + matches := re.FindStringSubmatch(word) + if matches == nil { + return nil + } + + // Ignore errors (opening the link via the OS can fail if the + // `os.openLink` config key references a command that doesn't exist, or + // that errors when called.) + _ = gui.c.OS().OpenLink(matches[1]) + return nil } From b9a75ee0edbe6c8316a94db4cb3dcd71a696f099 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 27 Mar 2024 11:32:31 +0100 Subject: [PATCH 4/5] Make links clickable in confirmation panels This is not opt-in, we do it always. I can't imagine a situation where we wouldn't want it. --- pkg/gui/global_handlers.go | 8 ++++++++ pkg/gui/keybindings.go | 6 ++++++ 2 files changed, 14 insertions(+) diff --git a/pkg/gui/global_handlers.go b/pkg/gui/global_handlers.go index c20b10ad7..c64a20a9e 100644 --- a/pkg/gui/global_handlers.go +++ b/pkg/gui/global_handlers.go @@ -109,6 +109,14 @@ func (gui *Gui) scrollDownConfirmationPanel() error { return nil } +func (gui *Gui) handleConfirmationClick() error { + if gui.Views.Confirmation.Editable { + return nil + } + + return gui.handleGenericClick(gui.Views.Confirmation) +} + func (gui *Gui) handleCopySelectedSideContextItemToClipboard() error { return gui.handleCopySelectedSideContextItemToClipboardWithTruncation(-1) } diff --git a/pkg/gui/keybindings.go b/pkg/gui/keybindings.go index 02405b9b6..9c4acd1ee 100644 --- a/pkg/gui/keybindings.go +++ b/pkg/gui/keybindings.go @@ -247,6 +247,12 @@ func (self *Gui) GetInitialKeybindings() ([]*types.Binding, []*gocui.ViewMouseBi Modifier: gocui.ModNone, Handler: self.scrollDownConfirmationPanel, }, + { + ViewName: "confirmation", + Key: gocui.MouseLeft, + Modifier: gocui.ModNone, + Handler: self.handleConfirmationClick, + }, { ViewName: "confirmation", Key: gocui.MouseWheelUp, From 5d509efe19b4f17b971470dadca6e1d18ea3c369 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 27 Mar 2024 13:38:44 +0100 Subject: [PATCH 5/5] Underline links in confirmation panels --- .../helpers/confirmation_helper.go | 28 ++++++++- .../helpers/confirmation_helper_test.go | 63 +++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 pkg/gui/controllers/helpers/confirmation_helper_test.go diff --git a/pkg/gui/controllers/helpers/confirmation_helper.go b/pkg/gui/controllers/helpers/confirmation_helper.go index 8a61a86e1..6cf5a1e90 100644 --- a/pkg/gui/controllers/helpers/confirmation_helper.go +++ b/pkg/gui/controllers/helpers/confirmation_helper.go @@ -215,7 +215,7 @@ func (self *ConfirmationHelper) CreatePopupPanel(ctx goContext.Context, opts typ confirmationView.RenderTextArea() } else { self.c.ResetViewOrigin(confirmationView) - self.c.SetViewContent(confirmationView, style.AttrBold.Sprint(opts.Prompt)) + self.c.SetViewContent(confirmationView, style.AttrBold.Sprint(underlineLinks(opts.Prompt))) } if err := self.setKeyBindings(cancel, opts); err != nil { @@ -228,6 +228,32 @@ func (self *ConfirmationHelper) CreatePopupPanel(ctx goContext.Context, opts typ return self.c.PushContext(self.c.Contexts().Confirmation) } +func underlineLinks(text string) string { + result := "" + remaining := text + for { + linkStart := strings.Index(remaining, "https://") + if linkStart == -1 { + break + } + + linkEnd := strings.IndexAny(remaining[linkStart:], " \n>") + if linkEnd == -1 { + linkEnd = len(remaining) + } else { + linkEnd += linkStart + } + underlinedLink := style.AttrUnderline.Sprint(remaining[linkStart:linkEnd]) + if strings.HasSuffix(underlinedLink, "\x1b[0m") { + // Replace the "all styles off" code with "underline off" code + underlinedLink = underlinedLink[:len(underlinedLink)-2] + "24m" + } + result += remaining[:linkStart] + underlinedLink + remaining = remaining[linkEnd:] + } + return result + remaining +} + func (self *ConfirmationHelper) setKeyBindings(cancel goContext.CancelFunc, opts types.CreatePopupPanelOpts) error { var onConfirm func() error if opts.HandleConfirmPrompt != nil { diff --git a/pkg/gui/controllers/helpers/confirmation_helper_test.go b/pkg/gui/controllers/helpers/confirmation_helper_test.go new file mode 100644 index 000000000..488c72710 --- /dev/null +++ b/pkg/gui/controllers/helpers/confirmation_helper_test.go @@ -0,0 +1,63 @@ +package helpers + +import ( + "testing" + + "github.com/gookit/color" + "github.com/stretchr/testify/assert" + "github.com/xo/terminfo" +) + +func Test_underlineLinks(t *testing.T) { + scenarios := []struct { + name string + text string + expectedResult string + }{ + { + name: "empty string", + text: "", + expectedResult: "", + }, + { + name: "no links", + text: "abc", + expectedResult: "abc", + }, + { + name: "entire string is a link", + text: "https://example.com", + expectedResult: "\x1b[4mhttps://example.com\x1b[24m", + }, + { + name: "link preceeded and followed by text", + text: "bla https://example.com xyz", + expectedResult: "bla \x1b[4mhttps://example.com\x1b[24m xyz", + }, + { + name: "more than one link", + text: "bla https://link1 blubb https://link2 xyz", + expectedResult: "bla \x1b[4mhttps://link1\x1b[24m blubb \x1b[4mhttps://link2\x1b[24m xyz", + }, + { + name: "link in angle brackets", + text: "See for details", + expectedResult: "See <\x1b[4mhttps://example.com\x1b[24m> for details", + }, + { + name: "link followed by newline", + text: "URL: https://example.com\nNext line", + expectedResult: "URL: \x1b[4mhttps://example.com\x1b[24m\nNext line", + }, + } + + oldColorLevel := color.ForceSetColorLevel(terminfo.ColorLevelMillions) + defer color.ForceSetColorLevel(oldColorLevel) + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + result := underlineLinks(s.text) + assert.Equal(t, s.expectedResult, result) + }) + } +}