Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Preserve selection when workspaces starts or stops
When a workspace starts the workspace row is replaced with a row for the
agent(s) and you have to select it again which is annoying (similarly
for when the workspace stops).  This preserves the selection when the
workspace transitions.
  • Loading branch information
code-asher committed Apr 26, 2023
commit a83b0bc0329000f94ee96e5f2bf73ac66fa59fe2
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,16 @@ import com.coder.gateway.sdk.v2.models.WorkspaceTransition
import java.util.UUID
import javax.swing.Icon

// TODO: Refactor to have a list of workspaces that each have agents. We
// present in the UI as a single flat list in the table (when there are no
// agents we display a row for the workspace) but still, a list of workspaces
// each with a list of agents might reflect reality more closely. When we
// iterate over the list we can add the workspace row if it has no agents
// otherwise iterate over the agents and then flatten the result.
data class WorkspaceAgentModel(
val workspaceID: UUID,
val workspaceName: String,
val name: String,
val name: String, // Name of the workspace OR the agent if this is for an agent.
val templateID: UUID,
val templateName: String,
val templateIconPath: String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -669,11 +669,9 @@ class CoderWorkspacesStepView(val setNextButtonEnabled: (Boolean) -> Unit) : Cod
}
}
withContext(Dispatchers.Main) {
val selectedWorkspace = tableOfWorkspaces.selectedObject?.name
val selectedWorkspace = tableOfWorkspaces.selectedObject
tableOfWorkspaces.listTableModel.items = ws.toList()
if (selectedWorkspace != null) {
tableOfWorkspaces.selectItem(selectedWorkspace)
}
tableOfWorkspaces.selectItem(selectedWorkspace)
}
}

Expand Down Expand Up @@ -923,15 +921,31 @@ class WorkspacesTableModel : ListTableModel<WorkspaceAgentModel>(
}

class WorkspacesTable : TableView<WorkspaceAgentModel>(WorkspacesTableModel()) {
fun selectItem(workspaceName: String?) {
if (workspaceName != null) {
this.items.forEachIndexed { index, workspaceAgentModel ->
if (workspaceAgentModel.name == workspaceName) {
selectionModel.addSelectionInterval(convertRowIndexToView(index), convertRowIndexToView(index))
// fix cell selection case
columnModel.selectionModel.addSelectionInterval(0, columnCount - 1)
}
}
/**
* Given either a workspace or an agent select in order of preference:
* 1. That same agent or workspace.
* 2. The first match for the workspace (workspace itself or first agent).
*/
fun selectItem(workspace: WorkspaceAgentModel?) {
val index = getNewSelection(workspace)
if (index > -1) {
selectionModel.addSelectionInterval(convertRowIndexToView(index), convertRowIndexToView(index))
// Fix cell selection case.
columnModel.selectionModel.addSelectionInterval(0, columnCount - 1)
}
}

private fun getNewSelection(oldSelection: WorkspaceAgentModel?): Int {
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming that returning -1 when not found is an explicit thing you're meant to do with a UI table?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is more related to lists in general than the table per se; we use an index to select an item and the various indexOf functions return -1 for no match so I did the same. But I also explicitly check for -1 and avoid selecting in that case so really we could return null for example if we wanted. I am not sure what would happen if we passed -1 for the selection index directly to the table.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested and it errors with invalid index.

It does have some other functions that will return a -1 (for example convertRowIndexToView returns a -1 if the row is not visible) so I think using -1 is generally the preferred pattern.

if (oldSelection == null) {
return -1
}
val index = listTableModel.items.indexOfFirst {
it.name == oldSelection.name && it.workspaceName == oldSelection.workspaceName
}
if (index > -1) {
return index
}
return listTableModel.items.indexOfFirst { it.workspaceName == oldSelection.workspaceName }
}

}
54 changes: 54 additions & 0 deletions src/test/groovy/CoderWorkspacesStepViewTest.groovy
Copy link
Member

Choose a reason for hiding this comment

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

👍 Nice test. I'm not a huge Groovy fan, but I do like the table-driven Spock DSL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same!

Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import com.coder.gateway.views.steps.WorkspacesTable
import spock.lang.Specification
import spock.lang.Unroll

@Unroll
class CoderWorkspacesStepViewTest extends Specification {
def "gets new selection"() {
given:
def table = new WorkspacesTable()
table.listTableModel.items = List.of(
// An off workspace.
DataGen.workspace("ws1", "ws1"),

// On workspaces.
DataGen.workspace("agent1", "ws2"),
DataGen.workspace("agent2", "ws2"),
DataGen.workspace("agent3", "ws3"),

// Another off workspace.
DataGen.workspace("ws4", "ws4"),

// In practice we do not list both agents and workspaces
// together but here test that anyway with an agent first and
// then with a workspace first.
DataGen.workspace("agent2", "ws5"),
DataGen.workspace("ws5", "ws5"),
DataGen.workspace("ws6", "ws6"),
DataGen.workspace("agent3", "ws6"),
)

expect:
table.getNewSelection(selected) == expected

where:
selected | expected
null | -1 // No selection.
DataGen.workspace("gone", "gone") | -1 // No workspace that matches.
DataGen.workspace("ws1", "ws1") | 0 // Workspace exact match.
DataGen.workspace("gone", "ws1") | 0 // Agent gone, select workspace.
DataGen.workspace("ws2", "ws2") | 1 // Workspace gone, select first agent.
DataGen.workspace("agent1", "ws2") | 1 // Agent exact match.
DataGen.workspace("agent2", "ws2") | 2 // Agent exact match.
DataGen.workspace("ws3", "ws3") | 3 // Workspace gone, select first agent.
DataGen.workspace("agent3", "ws3") | 3 // Agent exact match.
DataGen.workspace("gone", "ws4") | 4 // Agent gone, select workspace.
DataGen.workspace("ws4", "ws4") | 4 // Workspace exact match.
DataGen.workspace("agent2", "ws5") | 5 // Agent exact match.
DataGen.workspace("gone", "ws5") | 5 // Agent gone, another agent comes first.
DataGen.workspace("ws5", "ws5") | 6 // Workspace exact match.
DataGen.workspace("ws6", "ws6") | 7 // Workspace exact match.
DataGen.workspace("gone", "ws6") | 7 // Agent gone, workspace comes first.
DataGen.workspace("agent3", "ws6") | 8 // Agent exact match.
}
}