add suggestions for context and resources on the command bar #2285

wjiec · 2023-11-11T15:28:40Z

This PR adds autocompletion for context and resource types in the command bar, related Issue #2283.

It triggers the autocomplete after typing the Resource and then a space.

Looking forward to your guidance!

placintaalexandru · 2023-11-13T14:58:45Z

internal/view/app.go

+			}
+		}
+	default:
+		nss, err := a.factory.Client().ValidNamespaces()


wouldn't make sense to limit only at the search bar and within the given namespace?
from UX perspective I think we should allow only the resource type from current screen (only deployments, namespaces, stateful sets) and not mix all together

might be objective on this one, but I find it more organised this way

@placintaalexandru Thank you for weighting in! Could you clarify, don't think I understood your point??

hello @derailed
the original issue, according to my understanding, was to add search suggestions: for example if i am in the deployment view and type my-deployment, the search bar would suggest everything starting with my-deployment within the current namespace

Thank you for the clarification @placintaalexandru! I see what you are saying now. The original issue which Jayson implemented here is correct. I believe this issue relates to completing on the namespace when users enter a command ie dp xxx where xxx denotes suggestions from matching namespaces.
Best I can tell when the user activates a filter aka / we just want to filter the resources based on what the user types i.e no autocomplete. Does this make sense?

derailed

@wjiec Nice work!! Thank you so much Jayson for this PR!
This is great and something users have asked for.

I have some suggestions/improvements. I know I am asking a lot here ;(

We could push it as it is and I can help iterate on or if you want to drive this home I am game for that too...

derailed · 2023-11-21T17:20:26Z

internal/view/app.go

@@ -173,6 +172,49 @@ func (a *App) suggestCommand() model.SuggestionFunc {
 	}
 }

+func (a *App) suggestSubCommand(command string) []string {
+	cmds := strings.Split(command, " ")
+	if len(cmds[0]) == 0 || len(cmds) != 2 {


I have several suggestions here:

I think it might be a good idea to use strings.Fields instead to pick up potential typo ie po fred

Best to make this call a helper and take in dependencies aka context, ns maps. So we don't have to refetch as there could be lots there. I think we should provide apis ie ContextNames and NamespaceNames which could return a map of names??

Would be great to put some tests on this call.

I have several suggestions here:

I think it might be a good idea to use strings.Fields instead to pick up potential typo ie po fred

Best to make this call a helper and take in dependencies aka context, ns maps. So we don't have to refetch as there could be lots there. I think we should provide apis ie ContextNames and NamespaceNames which could return a map of names??

Would be great to put some tests on this call.

done

I'm trying to understand this suggestion, do you mean implementing it as a method in a struct while caching the contexts and the namespaces in the struct?

I'll provide test cases later

Right! I was originally thinking something like

func suggestSubCommand(cmd string, namespaces, contexts map[string]struct{}) []string { ... }

As this would make it cacheable and easier to test.
But... creating something like CmdParser/Interpreter struct would make better sense actually as we could use it for suggestions, hang convenience ie IsXrayCmd(), IsK9sCmd(), Suggest() etc...
This will allow us to have a single command interpreter for all command related things.

Would this make sense?

derailed · 2023-11-21T17:21:09Z

internal/view/app.go

+
+	var suggests []string
+	switch strings.ToLower(cmds[0]) {
+	case "cow", "q", "q!", "qa", "Q", "quit", "?", "h", "help", "a", "alias", "x", "xray", "dir":


Don't think this is necessary to check for those since we only care about context or something with this shape aka xxx ns ie len = 2 or am I missing it?
I am also thinking we should intro a command type, so we can hang convenience like IsContext() or IsTuple()?? These could be reused in several places and also could open up listing multiple nss i.e dp ns1 ns2 at some point in the future...

Don't think this is necessary to check for those since we only care about context or something with this shape aka xxx ns ie len = 2 or am I missing it?

There are two reasons for this, the first being that some of the commands in here have a second argument that is not ns, such as xray dp. The second is that for e.g. q these commands don't really need any more arguments, would it be weird if the user accidentally typed q x to see suggestions?

I am also thinking we should intro a command type, so we can hang convenience like IsContext() or IsTuple()?? These could be reused in several places and also could open up listing multiple nss i.e dp ns1 ns2 at some point in the future...

Yes, it also helps to manage these commands better. It may be a bit tricky to manage with resource commands?

Good point! And there might be others coming in the future that could break this code.
Thus I think the command struct parser/interpreter makes more and more sense to derive all these from a central place.

derailed · 2023-11-21T17:27:29Z

internal/view/app.go

+			}
+		}
+	default:
+		nss, err := a.factory.Client().ValidNamespaces()


@placintaalexandru Thank you for weighting in! Could you clarify, don't think I understood your point??

derailed · 2023-11-21T17:31:17Z

internal/view/app.go

+		}
+
+		for contextName := range ctxs {
+			if suggest, ok := shouldAddSuggest(cmds[1], contextName); ok {


Also it would be great to add fuzzy matching here (we do this with filter commands) so user could type po ed and suggest fred, bleed

Using fuzzy matching can result in very strange views. For example, there are these namespaces: kube-system, kube-storage, if I type: po ks it comes up with po ks[gray]kube-system[gray], or if I type po age it comes up with po age[gray]kube-storage[gray]. , I think this may be not understandable for users, what do you think?

Right! This could be a double edge sword indeed.
Perhaps using fuzzy logic and returning the top 3 scored suggestions could make sense?
I was looking at #2163 so perhaps checking for contains vs prefix would help in this case? but could also yield additional user headaches?

Perhaps outside the scope of this PR and we could experiment with this a bit and see how we could address this better in the future.

If you have other ideas, I am all ears!

derailed

@wjiec Thank you for these updates and questions!
I am on the fence here as we could just tune things a bit more and checkin as such as this will already help our users a all lot.
Perhaps in the background, we could experiment with other mechanics and see how we could help users further dealing with either bad typist or long entity names??

derailed · 2023-11-24T15:35:14Z

internal/view/app.go

@@ -173,6 +172,49 @@ func (a *App) suggestCommand() model.SuggestionFunc {
 	}
 }

+func (a *App) suggestSubCommand(command string) []string {
+	cmds := strings.Split(command, " ")
+	if len(cmds[0]) == 0 || len(cmds) != 2 {


Right! I was originally thinking something like

func suggestSubCommand(cmd string, namespaces, contexts map[string]struct{}) []string { ... }

As this would make it cacheable and easier to test.
But... creating something like CmdParser/Interpreter struct would make better sense actually as we could use it for suggestions, hang convenience ie IsXrayCmd(), IsK9sCmd(), Suggest() etc...
This will allow us to have a single command interpreter for all command related things.

Would this make sense?

derailed · 2023-11-24T15:45:13Z

internal/view/app.go

+
+	var suggests []string
+	switch strings.ToLower(cmds[0]) {
+	case "cow", "q", "q!", "qa", "Q", "quit", "?", "h", "help", "a", "alias", "x", "xray", "dir":


Good point! And there might be others coming in the future that could break this code.
Thus I think the command struct parser/interpreter makes more and more sense to derive all these from a central place.

derailed · 2023-11-24T16:01:06Z

internal/view/app.go

+		}
+
+		for contextName := range ctxs {
+			if suggest, ok := shouldAddSuggest(cmds[1], contextName); ok {


Right! This could be a double edge sword indeed.
Perhaps using fuzzy logic and returning the top 3 scored suggestions could make sense?
I was looking at #2163 so perhaps checking for contains vs prefix would help in this case? but could also yield additional user headaches?

Perhaps outside the scope of this PR and we could experiment with this a bit and see how we could address this better in the future.

If you have other ideas, I am all ears!

wjiec · 2023-11-25T14:05:10Z

Sorry for the late reply, I changed the signature of suggestSubCommand and added some test cases to it.

I think we should start by making this PR as concise as possible, it probably solves 90% of the problems already, although there are a few things that make people uncomfortable.

I couldn't agree more with your parser/interpreter idea, which makes it possible to configure independent suggestions for each command, and also provides more freedom to the user (suggestions via plugins?). I'd be happy to implement this later.

Fuzzy matching is also a very useful feature, and we might be able to provide suggestions by way of a dropdown? (with up down keypad). Sort of like how fig works in iterm2. And I found an example of using dropdown in tview:

https://github.com/rivo/tview/tree/master/demos/dropdown

derailed · 2023-11-26T14:25:36Z

internal/view/app_test.go

@@ -1,19 +1,46 @@
 // SPDX-License-Identifier: Apache-2.0
 // Copyright Authors of K9s

-package view_test
+package view


nit: In the project, we typically use xxx_test.go to test exported calls and xxx_int_test to test unexported calls.

derailed

@wjiec Thank you for the updates Jayson! Just a small pick...

…d#2285) * add suggestions for context and resources on the command bar * instead strings.Fields * cacheable and provide test cases

wjiec marked this pull request as draft November 11, 2023 15:31

wjiec force-pushed the feat/cmdbar-suggest branch from e82c94e to 4d93d53 Compare November 12, 2023 13:44

wjiec marked this pull request as ready for review November 12, 2023 13:44

derailed added enhancement New feature or request needs-review PR needs to be reviewed labels Nov 12, 2023

wjiec force-pushed the feat/cmdbar-suggest branch from 4d93d53 to 4b0ad25 Compare November 13, 2023 01:37

placintaalexandru reviewed Nov 13, 2023

View reviewed changes

derailed requested changes Nov 21, 2023

View reviewed changes

derailed added change-requested PR requires updates and removed needs-review PR needs to be reviewed labels Nov 21, 2023

wjiec force-pushed the feat/cmdbar-suggest branch from 4b0ad25 to 03f3011 Compare November 22, 2023 14:18

derailed reviewed Nov 24, 2023

View reviewed changes

wjiec added 3 commits November 25, 2023 21:19

add suggestions for context and resources on the command bar

2c77f94

instead strings.Fields

54a5d96

cacheable and provide test cases

4a4e0d2

wjiec force-pushed the feat/cmdbar-suggest branch from 03f3011 to 4a4e0d2 Compare November 25, 2023 13:50

derailed reviewed Nov 26, 2023

View reviewed changes

derailed merged commit fcfff57 into derailed:master Nov 27, 2023
1 check passed

wjiec mentioned this pull request Nov 27, 2023

fix namespace suggestion error on context switch #2315

Merged

wjiec deleted the feat/cmdbar-suggest branch November 27, 2023 02:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add suggestions for context and resources on the command bar #2285

add suggestions for context and resources on the command bar #2285

wjiec commented Nov 11, 2023

placintaalexandru Nov 13, 2023 •

edited

Loading

derailed Nov 21, 2023

placintaalexandru Nov 21, 2023 •

edited

Loading

derailed Nov 22, 2023

derailed left a comment

derailed Nov 21, 2023

wjiec Nov 22, 2023

derailed Nov 24, 2023

derailed Nov 21, 2023

wjiec Nov 22, 2023

derailed Nov 24, 2023

derailed Nov 21, 2023

derailed Nov 21, 2023

wjiec Nov 22, 2023

derailed Nov 24, 2023

derailed left a comment

derailed Nov 24, 2023

derailed Nov 24, 2023

derailed Nov 24, 2023

wjiec commented Nov 25, 2023

derailed Nov 26, 2023

derailed left a comment

add suggestions for context and resources on the command bar #2285

add suggestions for context and resources on the command bar #2285

Conversation

wjiec commented Nov 11, 2023

placintaalexandru Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

placintaalexandru Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derailed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derailed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjiec commented Nov 25, 2023

Choose a reason for hiding this comment

derailed left a comment

Choose a reason for hiding this comment

placintaalexandru Nov 13, 2023 •

edited

Loading

placintaalexandru Nov 21, 2023 •

edited

Loading