feat: add workspace app icons to tray window #86
Conversation
- Adds AgentAppViewModel to handle each button - Adds collapsible control components to handle the collapsing section - Adds Uuid type to work around issues with the built-in Guid type - Adds ModelMerge utility for merging lists with minimal updates to work around constant flashing in the UI
|
|
||
| // Sort by status green, red, gray, then by hostname. | ||
| agents.Sort((a, b) => | ||
| ModelUpdate.ApplyLists(Agents, agents, (a, b) => |
There was a problem hiding this comment.
I don't think this application will propagate the PropertyChanged event, which means that existing agents won't collapse new agents when they are expanded.
Having agent view models collapse other agent view models feels fragile. Could we have some state on this view model that controls which agent is expanded, rather than a bunch of IsExpanded bools on each agent view model that we need to toggle? Or, could we lift the routine that expands an agent up to this model, so that it can expand exactly one of them?
There was a problem hiding this comment.
+1, on the mac app we just modify isExpanded: UUID? to do this.
There was a problem hiding this comment.
I don't think this application will propagate the PropertyChanged event, which means that existing agents won't collapse new agents when they are expanded.
It won't propagate it, but every single agent that was created will get the event listener added exactly once when created.
There was a problem hiding this comment.
Oh, whoops, I missed that you are iterating over Agents not agents. You're right that each new agent gets the listener, but now there could be concurrent access to Agents while you are modifying them.
There was a problem hiding this comment.
There shouldn't be concurrent access either since we're doing all of this in the UI thread (PropertyChanged gets called immediately when setting an observable property, which can only happen in the UI thread), and all changes to Agents also happen in the UI thread.
I agree it's not pretty to do it this way, but I think the idea of passing down the ID of the currently expanded item to each child is also not pretty IMO. It works better in Swift since you can safely have a pointer to an ID shared between all AgentViewModels, but you can't do that in C# without using unsafe or wrapping the ID in a class and using that as a reference. I thought about implementing it with a binding but since we're not creating these view models in the XAML code we have to write the bindings ourself which is not pretty.
Since we're using an ItemsRepeater to render these agents, the data context changes and we can't access the TrayWindowViewModel to e.g. set the command handler for the button to be a method on the parent view model unless we pass TrayWindowViewModel to the child AgentViewModel, which is also not pretty.
Every method of doing this seems to suck in a different way unfortunately. The reason I went this way originally was so none of the AgentViewModels had to be aware of any of the other AgentViewModels.
There was a problem hiding this comment.
Spike and I discussed in a call and we've opted to:
- Create a new interface
IAgentExpanderHostwhich is implemented byTrayWindowViewModel - Pass the
TrayWindowViewModelas anIAgentExpanderHostinto all of theAgentViewModels - Have each of the
AgentViewModels call the method on the interface whenthis.IsExpandedchanges - The implementation on
TrayWindowViewModelwill collapse every other agent by callingagent.SetExpanded(false)
|
|
||
| public partial class CoderApiClient | ||
| { | ||
| public Task<WorkspaceAgent> GetWorkspaceAgent(string id, CancellationToken ct = default) |
There was a problem hiding this comment.
This would be safer if we accepted a UUID, rather than a string, which could totally change the endpoint we hit.
There was a problem hiding this comment.
I considered this but didn't want to add a dependency between CoderSdk and Vpn.Proto, and adding a new project for just a UUID type seems overkill. The Go codersdk package accepts string IDs for some methods as well, so I think it's fine.
AgentAppViewModelto handle each buttonUuidtype to work around issues with the built-inGuidtypeModelMergeutility for merging lists with minimal updates to work around constant flashing in the UITODO:
ModelMergeAgentAppViewModelsince I don't think they do anything$SESSION_TOKENreplacementKnown issues:
/icon/cursor.svgseems to be an SVG with an embedded PNG, which I guess can't be rendered properly by WinUIbuttons.mp4
Closes #50