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.

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

Support displaying images data from tui i #55

Merged
merged 19 commits into from
Sep 18, 2024

Conversation

eharris128
Copy link
Contributor

What

  • Return images from OnCommand in the images command package
  • Move the images model to the images command package
  • Support navigation to the boilerplate debug model

Why

  • Enable entering the tui mode for images from either the mint images --tui or the mint tui -> i use modes
  • Enables navigation among different models in tui mode

How Tested

  • mint images -> should not open tui mode
  • mint images --tui -> should open tui mode
  • mint tui -> opens tui mode -> i -> open image mode -> esc -> return to the home mode -> d -> open the debug view -> q -> quit the TUI

Open Issues

  • I turned down the logs that are output to the CLI after exiting either the standalone TUI or the multi-model TUI mode. Both of these modes still output the app level logs to the CLI after the TUI closes.
  • We want no logs to be output to the CLI after exiting TUI mode. This should be addressed in a follow up MR:
    image

General questions

  • Do we want to colocate the models associaqted with each command inside of the command package?
  • Given the images OnCommand outputs some data that the TUI model needs to ingest in order to perform an update, what is the preferred pattern to await the return of data from the call of OnCommand?
    • After fighting with race conditions, I took the path of modifying OnCommand in the images package to return images. My previous attempt had been to call ImageModel.Update (not exact reference), but because this call was returning before the OnCommand return, the final model returned inside of the previousImageModel.Update call (being the initial call performed by the Home Model 'i' key input handler ) did not retain the images data. Having some intermediate state handler inside of the images model was another approach, but this felt more confusing to maintain.

Signed-off-by: Evan Harris <[email protected]>
Signed-off-by: Evan Harris <[email protected]>
Signed-off-by: Evan Harris <[email protected]>
Signed-off-by: Evan Harris <[email protected]>
Signed-off-by: Evan Harris <[email protected]>
Signed-off-by: Evan Harris <[email protected]>
Signed-off-by: Evan Harris <[email protected]>
Signed-off-by: Evan Harris <[email protected]>
Signed-off-by: Evan Harris <[email protected]>
@eharris128 eharris128 changed the title Support opening displaying images data from tui i Support displaying images data from tui i Sep 16, 2024
@kcq
Copy link
Contributor

kcq commented Sep 17, 2024

Given the images OnCommand outputs some data that the TUI model needs to ingest in order to perform an update <- ideally it'll be good to figure out how to use the execution context for the TUI as a way for the business logic in OnCommand to do I/O.

For example, for the images command CLI.Action (in pkg/app/master/command/images/cli.go) creates the command's execution context like this:

xc := app.NewExecutionContext(
			Name,
			gcvalues.QuietCLIMode,
			gcvalues.OutputFormat)

Then it passes xc to OnCommand.

It'll be good to figure out how to extend the app.ExecutionContext interface/struct to support what the TUI needs.

@kcq
Copy link
Contributor

kcq commented Sep 17, 2024

Waiting for results in OnCommand is doable (at least, in a straightforward way) in some of the commands (e.g., images), but other commands are interactive (e.g., debug) and waiting there until OnCommand returns doesn't quite work.

)

// Model represents the state of the TUI.
type Model struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well... let's use a less Bubble Tea specific name... Let's call it TUI (type TUI struct). This way, somebody who doesn't know much about the Bubble Tea architecture and terminology will be able to understand what this code is for. Comments help too, of course. Calling the struct TUI will make it more obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted and addressed.

}

// InitialModel returns the initial state of the model.
func InitialModel(images map[string]crt.BasicImageInfo, standalone bool) *Model {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well... InitialTUI will make it easier to understand what it's for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted and addressed.

@kcq kcq merged commit 89f9cf2 into mintoolkit:master Sep 18, 2024
3 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants