Skip to content

revise recent projects flow to be less confusing #464

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Aug 19, 2024
Merged

revise recent projects flow to be less confusing #464

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a51beea
revise recent projects flow to be less confusing
bcpeinhardt Aug 14, 2024
c704fa5
swap hourglass for spinner
bcpeinhardt Aug 14, 2024
6aeb88d
make the link just start the workspace
bcpeinhardt Aug 15, 2024
7cd80fb
disable link when appropriate and show loading states
bcpeinhardt Aug 16, 2024
9c876af
remove underline for disabled link
bcpeinhardt Aug 16, 2024
cc07fac
re-add wrapping to description label
bcpeinhardt Aug 16, 2024
93101b3
add back status icon in error case
bcpeinhardt Aug 19, 2024
901c2c3
run linter
bcpeinhardt Aug 19, 2024
a6b196b
update changelog
bcpeinhardt Aug 19, 2024
3202bb8
decide icon in one place
bcpeinhardt Aug 19, 2024
3020473
use let instead of git add .
bcpeinhardt Aug 19, 2024
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
19 changes: 13 additions & 6 deletions src/main/kotlin/com/coder/gateway/views/CoderGatewayRecentWorkspaceConnectionsView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,14 +174,15 @@ class CoderGatewayRecentWorkspaceConnectionsView(private val setContentCallback:
val workspaceWithAgent = deployment?.items?.firstOrNull { it.workspace.name == workspaceName }
val status =
if (deploymentError != null) {
Pair(UIUtil.getErrorForeground(), deploymentError)
Triple(UIUtil.getErrorForeground(), deploymentError, UIUtil.getBalloonErrorIcon())
} else if (workspaceWithAgent != null) {
Pair(
Triple(
workspaceWithAgent.status.statusColor(),
workspaceWithAgent.status.description,
null
)
} else {
Pair(UIUtil.getContextHelpForeground(), "Querying workspace status...")
Triple(UIUtil.getContextHelpForeground(), "Querying workspace status...", AnimatedIcon.Default())
}
val gap =
if (top) {
Expand All @@ -201,19 +202,25 @@ class CoderGatewayRecentWorkspaceConnectionsView(private val setContentCallback:
label("").resizableColumn().align(AlignX.FILL)
}.topGap(gap)

connections.forEach { workspaceProjectIDE ->
val enableLinks = listOf(WorkspaceStatus.STOPPED, WorkspaceStatus.CANCELED, WorkspaceStatus.FAILED, WorkspaceStatus.STARTING, WorkspaceStatus.RUNNING).contains(workspaceWithAgent?.workspace?.latestBuild?.status)
val inLoadingState = listOf(WorkspaceStatus.STARTING, WorkspaceStatus.CANCELING, WorkspaceStatus.DELETING, WorkspaceStatus.STOPPING).contains(workspaceWithAgent?.workspace?.latestBuild?.status)
val enableLinks = listOf(WorkspaceStatus.STOPPED, WorkspaceStatus.CANCELED, WorkspaceStatus.FAILED, WorkspaceStatus.STARTING, WorkspaceStatus.RUNNING).contains(workspaceWithAgent?.workspace?.latestBuild?.status)
val inLoadingState = listOf(WorkspaceStatus.STARTING, WorkspaceStatus.CANCELING, WorkspaceStatus.DELETING, WorkspaceStatus.STOPPING).contains(workspaceWithAgent?.workspace?.latestBuild?.status)

// We only display an API error on the first workspace rather than duplicating it on each workspace.
if (deploymentError == null || showError) {
row {
if (inLoadingState) {
icon(AnimatedIcon.Default())
}
if (status.third != null) {
icon(status.third!!)
}
Copy link
Member

Choose a reason for hiding this comment

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

idk if this is more Kotlin-y but I think you can also do something like this over the assertion:

status.third?.let { icon (it) }

Admittedly untested though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I low key hate that they named a function let 💀 TBH I think the assertion here is clearer, but if that's the kotlin way then I'm game haha. I could also do a withoutNull I suppose.

Copy link
Member

@code-asher code-asher Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

withoutNull throws if null but here we are OK with the status potentially being null, so I think it would not quite fit here.

I think the main issue with assertions is that they bypass the safety provided to you by the language, which feels dangerous for an unclear benefit.

If you see a !! you need to reason about the entire code and determine that status.third is not going to be reassigned (and you need to do this every time related code is refactored), but we should let the machine do it for us. 😆 Admittedly it is pretty trivial in this case, but still!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I meant use withoutNull inside the if check, but you're totally right the let method is the best option here, I just wish they called it somewhere else lol.

Copy link
Member

Choose a reason for hiding this comment

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

ahaha yeah and I swear I have to look it up every time because I always forget it is called let

label("<html><body style='width:350px;'>" + status.second + "</html>").applyToComponent {
foreground = status.first
}
}
}

connections.forEach { workspaceProjectIDE ->
row {
icon(workspaceProjectIDE.ideProduct.icon)
if (enableLinks) {
Expand Down