Split UI from event handling #448
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Alex Goodman <wagoodman@gmail.com>
Benchmark Test ResultsBenchmark results from the latest changes vs base branch |
Signed-off-by: Alex Goodman <wagoodman@gmail.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
luhring
approved these changes
Jun 29, 2021
cmd/packages.go
Outdated
| ux := ui.Select(appConfig.CliOptions.Verbosity > 0, appConfig.Quiet) | ||
| return ux(errs, eventSubscription) | ||
| return eventLoop( | ||
| packagesExecWorker(args[0]), |
There was a problem hiding this comment.
nit: If we're passing in a specific arg, it could be a nice hint to the reader to identify the arg via an explicit parameter for packagesExec, rather than receive the whole slice and grab item 0 just in time.
| @@ -21,6 +21,7 @@ require ( | |||
| github.com/gookit/color v1.2.7 | |||
| github.com/hashicorp/go-multierror v1.1.0 | |||
| github.com/hashicorp/go-version v1.2.0 | |||
| github.com/k0kubun/go-ansi v0.0.0-20180517002512-3bf9e2903213 | |||
There was a problem hiding this comment.
Just double-checking that this was intended, since I couldn't remember if we're adding new ANSI logic here.
There was a problem hiding this comment.
I've used this package before and decided to use it for the hide/show of the cursor (was intended)
ui/event_handlers.go
Outdated
| const statusSet = common.SpinnerDotSet // SpinnerCircleOutlineSet | ||
| const completedStatus = "✔" // "●" | ||
| const statusSet = components.SpinnerDotSet // SpinnerCircleOutlineSet | ||
| const completedStatus = "✔" // "●" |
There was a problem hiding this comment.
Just curious: What is "●" for?
There was a problem hiding this comment.
I can remove this --it was used during early development when I was flopping between options
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
GijsCalis
pushed a commit
to GijsCalis/syft
that referenced
this pull request
Feb 19, 2024
* split UI from event handling Signed-off-by: Alex Goodman <wagoodman@gmail.com> * add event loop tests Signed-off-by: Alex Goodman <wagoodman@gmail.com> * use stereoscope cleanup function during signal handling Signed-off-by: Alex Goodman <alex.goodman@anchore.com> * correct error wrapping in packages cmd Signed-off-by: Alex Goodman <alex.goodman@anchore.com> * migrate ui event handlers to ui package Signed-off-by: Alex Goodman <alex.goodman@anchore.com> * clarify command worker input var + remove dead comments Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Today the event loop is coupled with the specific UI details, which is not ideal. This PR splits out those concerns, migrating the event loop from the
uipackage tocmdand extracting theui.UIfunction into an interface for setting up a UI, handling individual events from the event loop, and tearing down the UI. This allows for flow control, signal handling, and worker/event management to be shared, and the UI specifics to be implemented behind an interface --this allows for the ability to swap out the ETUI implementation for the logger implementation, but now without duplicating flow control logic.Closes #416