From 8bdf2668fd599aedf38d842a0d7b4a8812ef4ec0 Mon Sep 17 00:00:00 2001 From: Ben Durrans Date: Tue, 22 Apr 2025 12:11:21 +0100 Subject: [PATCH 1/3] fix: seperate title and message in progress bars The title must be given at the start of the progress bar and cannot change, whereas the message can change at any time throughout. This is to be inline with the limitations on progress bars imposed by the LSP. --- .../code_workflow/native_workflow.go | 39 ++++++++++++------- .../data_transformation_workflow.go | 3 +- pkg/ui/consoleui_test.go | 17 +++----- pkg/ui/progressbar.go | 38 +++++++++++------- pkg/ui/userinterface.go | 12 +++--- 5 files changed, 62 insertions(+), 47 deletions(-) diff --git a/pkg/local_workflows/code_workflow/native_workflow.go b/pkg/local_workflows/code_workflow/native_workflow.go index 08aab864d..4c88b235c 100644 --- a/pkg/local_workflows/code_workflow/native_workflow.go +++ b/pkg/local_workflows/code_workflow/native_workflow.go @@ -54,6 +54,13 @@ const ( type OptionalAnalysisFunctions func(string, func() *http.Client, *zerolog.Logger, configuration.Configuration, ui.UserInterface) (*sarif.SarifResponse, *scan.ResultMetaData, error) +func NewTrackerFactory(userInterface ui.UserInterface, logger *zerolog.Logger) scan.TrackerFactory { + return ProgressTrackerFactory{ + userInterface: userInterface, + logger: logger, + } +} + type ProgressTrackerFactory struct { userInterface ui.UserInterface logger *zerolog.Logger @@ -61,22 +68,24 @@ type ProgressTrackerFactory struct { func (p ProgressTrackerFactory) GenerateTracker() scan.Tracker { return &ProgressTrackerAdapter{ - bar: p.userInterface.NewProgressBar(), - logger: p.logger, + userInterface: p.userInterface, + logger: p.logger, } } type ProgressTrackerAdapter struct { - bar ui.ProgressBar - logger *zerolog.Logger + userInterface ui.UserInterface + logger *zerolog.Logger + bar ui.ProgressBar } -func (p ProgressTrackerAdapter) Begin(title, message string) { - if len(message) > 0 { - p.bar.SetTitle(title + " - " + message) - } else { - p.bar.SetTitle(title) +func (p *ProgressTrackerAdapter) Begin(title, message string) { + if p.bar != nil { + p.logger.Error().Msg("progress tracker already begun") + return } + p.bar = p.userInterface.NewProgressBar(title) + p.bar.SetMessage(message) err := p.bar.UpdateProgress(ui.InfiniteProgress) if err != nil { @@ -84,8 +93,11 @@ func (p ProgressTrackerAdapter) Begin(title, message string) { } } -func (p ProgressTrackerAdapter) End(message string) { - p.bar.SetTitle(message) +func (p *ProgressTrackerAdapter) End(message string) { + if p.bar == nil { + return + } + p.bar.SetMessage(message) err := p.bar.Clear() if err != nil { p.logger.Err(err).Msg("Failed to clear progress") @@ -205,10 +217,7 @@ func defaultAnalyzeFunction(path string, httpClientFunc func() *http.Client, log localConfiguration: config, } - progressFactory := ProgressTrackerFactory{ - userInterface: userInterface, - logger: logger, - } + progressFactory := NewTrackerFactory(userInterface, logger) analysisOptions := []codeclient.AnalysisOption{} diff --git a/pkg/local_workflows/data_transformation_workflow.go b/pkg/local_workflows/data_transformation_workflow.go index b381d95b6..65b4c440e 100644 --- a/pkg/local_workflows/data_transformation_workflow.go +++ b/pkg/local_workflows/data_transformation_workflow.go @@ -40,8 +40,7 @@ func dataTransformationEntryPoint(invocationCtx workflow.InvocationContext, inpu return output, nil } - progress := invocationCtx.GetUserInterface().NewProgressBar() - progress.SetTitle("Transforming data") + progress := invocationCtx.GetUserInterface().NewProgressBar("Transforming data") progressError := progress.UpdateProgress(ui.InfiniteProgress) if progressError != nil { logger.Err(progressError).Msgf("Error when setting progress") diff --git a/pkg/ui/consoleui_test.go b/pkg/ui/consoleui_test.go index 644464f24..69df47de8 100644 --- a/pkg/ui/consoleui_test.go +++ b/pkg/ui/consoleui_test.go @@ -16,8 +16,7 @@ func Test_ProgressBar_Spinner(t *testing.T) { var err error writer := &bytes.Buffer{} - bar := newProgressBar(writer, SpinnerType, false) - bar.SetTitle("Hello") + bar := newProgressBar(writer, "Hello", SpinnerType, false) err = bar.UpdateProgress(0) assert.NoError(t, err) @@ -40,8 +39,7 @@ func Test_ProgressBar_Spinner_Infinite(t *testing.T) { var err error writer := &bytes.Buffer{} - bar := newProgressBar(writer, SpinnerType, false) - bar.SetTitle("Hello") + bar := newProgressBar(writer, "Hello", SpinnerType, false) err = bar.UpdateProgress(InfiniteProgress) assert.NoError(t, err) @@ -64,8 +62,7 @@ func Test_ProgressBar_Bar(t *testing.T) { var err error writer := &bytes.Buffer{} - bar := newProgressBar(writer, BarType, false) - bar.SetTitle("Hello") + bar := newProgressBar(writer, "Hello", BarType, false) err = bar.UpdateProgress(0) assert.NoError(t, err) @@ -88,8 +85,7 @@ func Test_ProgressBar_Unknown(t *testing.T) { var err error writer := &bytes.Buffer{} - bar := newProgressBar(writer, "Unknown", false) - bar.SetTitle("Hello") + bar := newProgressBar(writer, "Hello", "Unknown", false) err = bar.UpdateProgress(0) assert.NoError(t, err) @@ -120,11 +116,10 @@ func Test_DefaultUi(t *testing.T) { stdin.WriteString(name + "\n") ui := newConsoleUi(stdin, stdout, stderr) - bar := ui.NewProgressBar() + bar := ui.NewProgressBar("A Title") assert.NotNil(t, bar) // the bar will not render since the writer is not a TTY - bar.SetTitle("Hello") err := bar.UpdateProgress(InfiniteProgress) assert.NoError(t, err) @@ -161,7 +156,7 @@ func Test_OutputError(t *testing.T) { t.Run("Error Catalog error", func(t *testing.T) { err := snyk.NewBadRequestError("If you carry on like this you will ensure the wrath of OWASP, the God of Code Injection.") - err.Links = []string{"http://example.com/docs"} + err.Links = []string{"https://example.com/docs"} lipgloss.SetColorProfile(termenv.TrueColor) uiError := ui.OutputError(err) diff --git a/pkg/ui/progressbar.go b/pkg/ui/progressbar.go index d857091ee..3c060ec83 100644 --- a/pkg/ui/progressbar.go +++ b/pkg/ui/progressbar.go @@ -29,28 +29,29 @@ const ( // It is used to show the progress of some running task (or multiple). // Example (Infinite Progress without a value): // -// var pBar ProgressBar = ui.DefaultUi().NewProgressBar() +// pBar := ui.DefaultUi().NewProgressBar("CLI Downloader") // defer pBar.Clear() -// pBar.SetTitle("Downloading...") +// pBar.SetMessage("Downloading...") // _ = pBar.UpdateProgress(ui.InfiniteProgress) // // Example (with a value): // -// var pBar ProgressBar = ui.DefaultUi().NewProgressBar() +// pBar := ui.DefaultUi().NewProgressBar("CLI Downloader") // defer pBar.Clear() -// pBar.SetTitle("Downloading...") +// pBar.SetMessage("Downloading...") // for i := 0; i <= 50; i++ { // pBar.UpdateProgress(float64(i) / 100.0) // time.Sleep(time.Millisecond * 50) // } // -// pBar.SetTitle("Installing...") +// pBar.SetMessage("Installing...") // for i := 50; i <= 100; i++ { // pBar.UpdateProgress(float64(i) / 100.0) // time.Sleep(time.Millisecond * 50) // } // -// The title can be changed in the middle of the progress bar. +// The message can be changed in the middle of the progress bar. +// In some implementations no progress bar will be shown until the first UpdateProgress is called. type ProgressBar interface { // UpdateProgress updates the state of the progress bar. // The argument `progress` should be a float64 between 0 and 1, @@ -58,9 +59,10 @@ type ProgressBar interface { // Returns an error if the update operation fails. UpdateProgress(progress float64) error - // SetTitle sets the title of the progress bar, which is displayed next to the bar. - // The title provides context or description for the operation that is being tracked. - SetTitle(title string) + // SetMessage sets the message of the progress bar, which is displayed next to the bar. + // The message is displayed alongside the title passed in by `NewProgressBar(title string)`. + // The message provides context or description for the operation that is being tracked. + SetMessage(message string) // Clear removes the progress bar from the terminal. // Returns an error if the clearing operation fails. @@ -70,11 +72,14 @@ type ProgressBar interface { type emptyProgressBar struct{} func (emptyProgressBar) UpdateProgress(float64) error { return nil } -func (emptyProgressBar) SetTitle(string) {} +func (emptyProgressBar) SetMessage(string) {} func (emptyProgressBar) Clear() error { return nil } -func newProgressBar(writer io.Writer, t ProgressType, animated bool) *consoleProgressBar { - p := &consoleProgressBar{writer: writer} +func newProgressBar(writer io.Writer, title string, t ProgressType, animated bool) *consoleProgressBar { + p := &consoleProgressBar{ + writer: writer, + title: title, + } p.active.Store(true) p.animationRunning = false p.progressType = t @@ -85,6 +90,7 @@ func newProgressBar(writer io.Writer, t ProgressType, animated bool) *consolePro type consoleProgressBar struct { writer io.Writer title string + message string state int progress atomic.Pointer[float64] active atomic.Bool @@ -110,7 +116,7 @@ func (p *consoleProgressBar) UpdateProgress(progress float64) error { return nil } -func (p *consoleProgressBar) SetTitle(title string) { p.title = title } +func (p *consoleProgressBar) SetMessage(message string) { p.message = message } func (p *consoleProgressBar) Clear() error { if !p.active.Load() { @@ -141,6 +147,12 @@ func (p *consoleProgressBar) update() { if len(p.title) > 0 { progressString += p.title + if len(p.message) > 0 { + progressString += " - " + } + } + if len(p.message) > 0 { + progressString += p.message } p.state++ diff --git a/pkg/ui/userinterface.go b/pkg/ui/userinterface.go index a296ef1a7..4e9180e5d 100644 --- a/pkg/ui/userinterface.go +++ b/pkg/ui/userinterface.go @@ -21,7 +21,7 @@ import ( type UserInterface interface { Output(output string) error OutputError(err error, opts ...Opts) error - NewProgressBar() ProgressBar + NewProgressBar(title string) ProgressBar Input(prompt string) (string, error) } @@ -37,10 +37,10 @@ func newConsoleUi(in io.Reader, out io.Writer, err io.Writer) UserInterface { reader: bufio.NewReader(in), } - defaultUi.progressBarFactory = func() ProgressBar { + defaultUi.progressBarFactory = func(title string) ProgressBar { if stderr, ok := err.(*os.File); ok { if isatty.IsTerminal(stderr.Fd()) || isatty.IsCygwinTerminal(stderr.Fd()) { - return newProgressBar(err, SpinnerType, true) + return newProgressBar(err, title, SpinnerType, true) } } @@ -53,7 +53,7 @@ func newConsoleUi(in io.Reader, out io.Writer, err io.Writer) UserInterface { type consoleUi struct { writer io.Writer errorWriter io.Writer - progressBarFactory func() ProgressBar + progressBarFactory func(title string) ProgressBar reader *bufio.Reader } @@ -101,8 +101,8 @@ func (ui *consoleUi) OutputError(err error, opts ...Opts) error { return utils.ErrorOf(fmt.Fprintln(ui.errorWriter, err.Error())) } -func (ui *consoleUi) NewProgressBar() ProgressBar { - return ui.progressBarFactory() +func (ui *consoleUi) NewProgressBar(title string) ProgressBar { + return ui.progressBarFactory(title) } func (ui *consoleUi) Input(prompt string) (string, error) { From 899cd850187b4cfdf02b57eb3557878b6bd08d20 Mon Sep 17 00:00:00 2001 From: Ben Durrans Date: Tue, 22 Apr 2025 13:09:52 +0100 Subject: [PATCH 2/3] chore: generate mocks --- pkg/mocks/progressbar.go | 12 ++++++------ pkg/mocks/userinterface.go | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/mocks/progressbar.go b/pkg/mocks/progressbar.go index 660638d7c..7c673995b 100644 --- a/pkg/mocks/progressbar.go +++ b/pkg/mocks/progressbar.go @@ -47,16 +47,16 @@ func (mr *MockProgressBarMockRecorder) Clear() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Clear", reflect.TypeOf((*MockProgressBar)(nil).Clear)) } -// SetTitle mocks base method. -func (m *MockProgressBar) SetTitle(title string) { +// SetMessage mocks base method. +func (m *MockProgressBar) SetMessage(message string) { m.ctrl.T.Helper() - m.ctrl.Call(m, "SetTitle", title) + m.ctrl.Call(m, "SetMessage", message) } -// SetTitle indicates an expected call of SetTitle. -func (mr *MockProgressBarMockRecorder) SetTitle(title interface{}) *gomock.Call { +// SetMessage indicates an expected call of SetMessage. +func (mr *MockProgressBarMockRecorder) SetMessage(message interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetTitle", reflect.TypeOf((*MockProgressBar)(nil).SetTitle), title) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetMessage", reflect.TypeOf((*MockProgressBar)(nil).SetMessage), message) } // UpdateProgress mocks base method. diff --git a/pkg/mocks/userinterface.go b/pkg/mocks/userinterface.go index 6b3e44b89..8adc2accc 100644 --- a/pkg/mocks/userinterface.go +++ b/pkg/mocks/userinterface.go @@ -50,17 +50,17 @@ func (mr *MockUserInterfaceMockRecorder) Input(prompt interface{}) *gomock.Call } // NewProgressBar mocks base method. -func (m *MockUserInterface) NewProgressBar() ui.ProgressBar { +func (m *MockUserInterface) NewProgressBar(title string) ui.ProgressBar { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "NewProgressBar") + ret := m.ctrl.Call(m, "NewProgressBar", title) ret0, _ := ret[0].(ui.ProgressBar) return ret0 } // NewProgressBar indicates an expected call of NewProgressBar. -func (mr *MockUserInterfaceMockRecorder) NewProgressBar() *gomock.Call { +func (mr *MockUserInterfaceMockRecorder) NewProgressBar(title interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewProgressBar", reflect.TypeOf((*MockUserInterface)(nil).NewProgressBar)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewProgressBar", reflect.TypeOf((*MockUserInterface)(nil).NewProgressBar), title) } // Output mocks base method. From 4268109e0ab55526fd811f2af3c7b165bf1f13d8 Mon Sep 17 00:00:00 2001 From: Ben Durrans Date: Tue, 22 Apr 2025 13:21:39 +0100 Subject: [PATCH 3/3] fix: don't export unnecessary structs `SetMessage` and `Clear` usage clarification --- .../code_workflow/native_workflow.go | 14 +++++++------- pkg/ui/progressbar.go | 5 +++-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/pkg/local_workflows/code_workflow/native_workflow.go b/pkg/local_workflows/code_workflow/native_workflow.go index 0329a8a72..ce755b516 100644 --- a/pkg/local_workflows/code_workflow/native_workflow.go +++ b/pkg/local_workflows/code_workflow/native_workflow.go @@ -54,31 +54,31 @@ const ( type OptionalAnalysisFunctions func(string, func() *http.Client, *zerolog.Logger, configuration.Configuration, ui.UserInterface) (*sarif.SarifResponse, *scan.ResultMetaData, error) func NewTrackerFactory(userInterface ui.UserInterface, logger *zerolog.Logger) scan.TrackerFactory { - return ProgressTrackerFactory{ + return progressTrackerFactory{ userInterface: userInterface, logger: logger, } } -type ProgressTrackerFactory struct { +type progressTrackerFactory struct { userInterface ui.UserInterface logger *zerolog.Logger } -func (p ProgressTrackerFactory) GenerateTracker() scan.Tracker { - return &ProgressTrackerAdapter{ +func (p progressTrackerFactory) GenerateTracker() scan.Tracker { + return &progressTrackerAdapter{ userInterface: p.userInterface, logger: p.logger, } } -type ProgressTrackerAdapter struct { +type progressTrackerAdapter struct { userInterface ui.UserInterface logger *zerolog.Logger bar ui.ProgressBar } -func (p *ProgressTrackerAdapter) Begin(title, message string) { +func (p *progressTrackerAdapter) Begin(title, message string) { if p.bar != nil { p.logger.Error().Msg("progress tracker already begun") return @@ -92,7 +92,7 @@ func (p *ProgressTrackerAdapter) Begin(title, message string) { } } -func (p *ProgressTrackerAdapter) End(message string) { +func (p *progressTrackerAdapter) End(message string) { if p.bar == nil { return } diff --git a/pkg/ui/progressbar.go b/pkg/ui/progressbar.go index 3c060ec83..cb0e76574 100644 --- a/pkg/ui/progressbar.go +++ b/pkg/ui/progressbar.go @@ -62,9 +62,10 @@ type ProgressBar interface { // SetMessage sets the message of the progress bar, which is displayed next to the bar. // The message is displayed alongside the title passed in by `NewProgressBar(title string)`. // The message provides context or description for the operation that is being tracked. + // Set to "" to clear the message. SetMessage(message string) - // Clear removes the progress bar from the terminal. + // Clear removes the progress bar from the user interface. // Returns an error if the clearing operation fails. Clear() error } @@ -101,7 +102,7 @@ type consoleProgressBar struct { func (p *consoleProgressBar) UpdateProgress(progress float64) error { if !p.active.Load() { - return fmt.Errorf("progress not active") + return fmt.Errorf("progress bar not active") } p.progress.Store(&progress)