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

feat(app): add Go Back and Last View #2799

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,8 @@ K9s uses aliases to navigate most K8s resources.
| To view and switch to another Kubernetes context (Pod view) | `:`ctx⏎ | |
| To view and switch directly to another Kubernetes context (Last used view) | `:`ctx context-name⏎ | |
| To view and switch to another Kubernetes namespace | `:`ns⏎ | |
| To switch between the last active view/command (like how "cd -" works) | `-` | Navigation that adds breadcrumbs to the bottom are not commands |
| To go back through previous views/commands | `b` | Same as above |
Copy link
Owner

Choose a reason for hiding this comment

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

I think 'b' is already in use in some views aka to run benchmarks ;(
Perhaps we can use '-' as the last 2 cmd toggle as you have here but then useuse '<' and '>' to nav the command history?

| To view all saved resources | `:`screendump or sd⏎ | |
| To delete a resource (TAB and ENTER to confirm) | `ctrl-d` | |
| To kill a resource (no confirmation dialog, equivalent to kubectl delete --now) | `ctrl-k` | |
Expand Down
22 changes: 21 additions & 1 deletion internal/model/history.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,34 @@ func NewHistory(limit int) *History {
}
}

func (h *History) Pop() string {
// Last returns the most recent history item
func (h *History) Last() string {
if h.Empty() {
return ""
}

return h.commands[0]
}

// Pop removes the most recent history item and returns a bool if the list changed.
// Optional argument specifies how many to remove from the history
func (h *History) Pop(n ...int) bool {
if len(h.commands) == 0 {
return false
}

count := 1
if len(n) > 1 {
// only one argument is expected
return false
} else if len(n) == 1 {
Copy link
Owner

Choose a reason for hiding this comment

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

You are done here no need for the else clause.

Copy link
Owner

Choose a reason for hiding this comment

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

Also not keen on the vararg with only one option. Best to leave Pop as such and introduce PopN(n int)

count = n[0]
}

h.commands = h.commands[count:]
return true
}

// List returns the current command history.
func (h *History) List() []string {
return h.commands
Expand Down
1 change: 1 addition & 0 deletions internal/ui/key.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ const (
KeySlash = 47
KeyColon = 58
KeySpace = 32
KeyDash = 45 //or minus for those searching in the code
)

// Define Shift Keys.
Expand Down
50 changes: 50 additions & 0 deletions internal/view/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,8 @@ func (a *App) bindKeys() {
tcell.KeyCtrlE: ui.NewSharedKeyAction("ToggleHeader", a.toggleHeaderCmd, false),
tcell.KeyCtrlG: ui.NewSharedKeyAction("toggleCrumbs", a.toggleCrumbsCmd, false),
ui.KeyHelp: ui.NewSharedKeyAction("Help", a.helpCmd, false),
ui.KeyB: ui.NewSharedKeyAction("Go Back", a.previousView, false),
ui.KeyDash: ui.NewSharedKeyAction("Last View", a.lastView, false),
tcell.KeyCtrlA: ui.NewSharedKeyAction("Aliases", a.aliasCmd, false),
tcell.KeyEnter: ui.NewKeyAction("Goto", a.gotoCmd, false),
tcell.KeyCtrlC: ui.NewKeyAction("Quit", a.quitCmd, false),
Expand Down Expand Up @@ -691,6 +693,54 @@ func (a *App) helpCmd(evt *tcell.EventKey) *tcell.EventKey {
return nil
}

// previousView returns to the view prior to the current one in the history
func (a *App) previousView(evt *tcell.EventKey) *tcell.EventKey {
Copy link
Owner

Choose a reason for hiding this comment

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

This should really be named previousCmd to avoid confusion

if evt != nil && evt.Rune() == rune(ui.KeyB) && a.Prompt().InCmdMode() {
return evt
}
cmds := a.cmdHistory.List()
if !(len(cmds) > 1) {
Copy link
Owner

Choose a reason for hiding this comment

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

A dialog here feels a bit heavy, I think best to use a flash for this instance.
Also not keen on !(len(cmds) > 1. This is len(cmds) <=1 return.
But... as noted below better to defer these calls to cmdHistory.

Copy link
Owner

Choose a reason for hiding this comment

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

Additionally all these should be on cmdHistory apis. ie the logic for toggle last view and fwd/backwd should be methods on the history and not view logic imho i.ie toggleLastCmd, prevCmd, nextCmd, etc..

Copy link
Owner

Choose a reason for hiding this comment

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

Additionally if we surfaced a cmdHistory view all the necessary apis would be already there.

dialog.ShowError(
a.Styles.Dialog(),
a.Content.Pages,
"Can't go back any further")
return evt
} else {
previousCmd := cmds[1]
a.cmdHistory.Pop()
a.gotoResource(previousCmd, "", true)
a.ResetCmd()
}

return nil
}

// lastView switches between the last view and this one a la `cd -`
func (a *App) lastView(evt *tcell.EventKey) *tcell.EventKey {
Copy link
Owner

Choose a reason for hiding this comment

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

As above these are commands and not views since sub-views are not part of the cmd history.

if evt != nil && evt.Rune() == ui.KeyDash && a.Prompt().InCmdMode() {
return evt
}
cmds := a.cmdHistory.List()
if !(len(cmds) > 1) {
dialog.ShowError(
a.Styles.Dialog(),
a.Content.Pages,
"No previous view to switch to")
return evt
} else {
current := cmds[0]
last := cmds[1]
a.gotoResource(last, "", true)
// remove current page and last page
a.cmdHistory.Pop(2)
// re add in opposite order
a.cmdHistory.Push(current)
a.cmdHistory.Push(last)
}

return nil
}

func (a *App) aliasCmd(evt *tcell.EventKey) *tcell.EventKey {
if a.Content.Top() != nil && a.Content.Top().Name() == aliasTitle {
a.Content.Pop()
Expand Down
2 changes: 1 addition & 1 deletion internal/view/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ func TestAppNew(t *testing.T) {
a := view.NewApp(mock.NewMockConfig())
_ = a.Init("blee", 10)

assert.Equal(t, 12, a.GetActions().Len())
assert.Equal(t, 14, a.GetActions().Len())
}
2 changes: 1 addition & 1 deletion internal/view/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ func (c *Command) exec(p *cmd.Interpreter, gvr client.GVR, comp model.Component,
log.Error().Msg(string(debug.Stack()))

p := cmd.NewInterpreter("pod")
if cmd := c.app.cmdHistory.Pop(); cmd != "" {
if cmd := c.app.cmdHistory.Last(); cmd != "" {
p = p.Reset(cmd)
}
err = c.run(p, "", true)
Expand Down