feat(sort): allow sorting any column with top like navigation #2739
Conversation
|
@derailed Any feedback on this? Since this is a long standing issue with a previous PR, if the proposed implementation is bad in any way, please provide some insights on how we should go on implementing it. As it is really useful to be able to sort by any column quickly. |
|
@ventsislav-georgiev I think this feature is great. Let's resolve the conflict and I'll take a peek... |
ventsislav-georgiev
force-pushed
the
sort-on-any-column
branch
from
2a2c1ff to
2a46d90
Compare
|
@derailed Thanks, conflict resolved. |
|
ping @derailed |
ventsislav-georgiev
force-pushed
the
sort-on-any-column
branch
from
2a46d90 to
1fa9c61
Compare
|
@derailed Any chance we could merge this? |
|
@ventsislav-georgiev Looks like we have conflicts. Can you take a peek? |
ventsislav-georgiev
force-pushed
the
sort-on-any-column
branch
3 times, most recently
from
a3c7948 to
08d8cbc
Compare
|
@derailed rebased. |
ventsislav-georgiev
force-pushed
the
sort-on-any-column
branch
from
08d8cbc to
0461759
Compare
|
@derailed rebased again. |
ventsislav-georgiev
force-pushed
the
sort-on-any-column
branch
from
0461759 to
9c088e9
Compare
|
@derailed rebased yet again. |
| @@ -288,6 +288,26 @@ func (t *Table) SetSortCol(name string, asc bool) { | |||
| t.setSortCol(model1.SortColumn{Name: name, ASC: asc}) | |||
| } | |||
|
|
|||
| func (t *Table) GetSortCol() (string, bool) { | |||
| return t.sortCol.Name, t.sortCol.ASC | ||
| } | ||
|
|
||
| func (t *Table) isVisible(h model1.HeaderColumn) bool { |
There was a problem hiding this comment.
Nice refactor! I think this should also check for hide/wide attribute on the column.
Also we should reuse this call on buildRow below.
| @@ -15,6 +15,16 @@ import ( | |||
| "k8s.io/apimachinery/pkg/runtime" | |||
| ) | |||
|
|
|||
| type ( | |||
| // SortChange changes the column on which to sort | |||
| SortChange bool | |||
There was a problem hiding this comment.
I think this could be an int type using next, prev = +1, -1?
| @@ -210,6 +210,9 @@ func (t *Table) bindKeys() { | |||
| tcell.KeyCtrlW: ui.NewKeyAction("Toggle Wide", t.toggleWideCmd, false), | |||
| ui.KeyShiftN: ui.NewKeyAction("Sort Name", t.SortColCmd(nameCol, true), false), | |||
| ui.KeyShiftA: ui.NewKeyAction("Sort Age", t.SortColCmd(ageCol, true), false), | |||
| ui.KeyQ: ui.NewKeyAction("Reverse Sort Order", t.SortInvertCmd, false), | |||
There was a problem hiding this comment.
I think Q is poor choice for the invert sort. This is on me since the keyspace is limited.
Here is a thought... What if we just use the proposed PR and eliminate the canned sort keys.
I think there are several benefits here:
- Frees up the key space
- Allows us to use the defacto
Ror 'I' to reverse on the selected sort column - This method becomes the standard k9s sort i.e solely 3 keys aka '<|>|R' and we can reclaim everything else
and lighten the cognitive load in the process...
Do you think that would make sense?
| sortColIdx := -1 | ||
| newSortColIdx := -1 | ||
| prevSortColIdx := -1 | ||
| header := t.GetModel().Peek().GetHeader() |
There was a problem hiding this comment.
This is not ideal as we realize the model twice here and later in refresh which would be expensive.
So I think we should pass in the model to something like doRefresh(*model1.Data) vs calling Refresh once the sort is set.
There was a problem hiding this comment.
Also this should call setMSort(true) to tell the table manual sorting is now in effect.
| continue | ||
| } | ||
|
|
||
| if h.Name == sortCol { |
There was a problem hiding this comment.
What happens if sortCol is not set? As it stands we would scan the header twice and yield nothing
I think it might be best to list out all visible header columns first.
Get the current sort column index if set and just go +1 or -1 based on that.
Bonus: if we are out of range we can do a circular buffer where pressing < on the first col yields the last col and > in the last col bumps you back to the first one.
Updated the PR from @mlosicki #1256
Related to: #1000