Conversation
mmp
left a comment
mmp
left a comment
There was a problem hiding this comment.
Looks really nice overall and sorry for the delay in the review (especially since I think you will have some merging pain due to pkg/ going away.)
One general thought/comment is that this seems reasonable to add the core functionality but one could start to say "hm, we should probably ban their IP address for this sim at least so they don't try to immediately reconnect." But then someone could use a VPN or whatever if they wanted to get around that. Though I would like to avoid getting into that sort of arms race.
| return | ||
| } | ||
|
|
||
| if len(cmd) >= 3 && cmd[:2] == "*K" { |
There was a problem hiding this comment.
Could you switch *K to be .KICK? I'd like to keep the stuff that aren't real-world STARS commands to all start with ..
| // Any signed-in controller can kick others (no instructor/RPO restriction) | ||
|
|
||
| // Verify the target controller exists and is signed in | ||
| if !slices.Contains(ctx.Client.State.HumanControllers, targetTCP) { |
There was a problem hiding this comment.
As a general statement, this sort of check is better to perform on the server side: since the user's view of the current state of the world in ctx.Client.State is only updated once a second (and there's some network latency when it's sent), it's usually slightly out of date and so it's possible that the controller has just signed out. It's harmless in this case, but in that you need this check on the server side anyway, might as well remove this one.
There was a problem hiding this comment.
Where would this be better suited?
| s.State.Controllers[tcp] = s.SignOnPositions[tcp] | ||
| s.State.HumanControllers = append(s.State.HumanControllers, tcp) | ||
|
|
||
| // Set instructor status if the flag is true |
There was a problem hiding this comment.
Is this related to the kick command? If not it should probably be in a separate PR. (Is there a bug this is fixing?)
| return ErrNoSimForControllerToken | ||
| } else { | ||
| return s.ChangeControlPosition(ctrl.tcp, cs.TCP, cs.KeepTracks) | ||
| oldTCP := ctrl.tcp |
| // Any signed-in controller can kick others (no instructor/RPO restriction) | ||
|
|
||
| // Find the target controller in the same sim | ||
| var targetToken string |
There was a problem hiding this comment.
I think you can get rid of the for loop and if test and just do targetToken := adminCtrl.asim.controllersByTCP[targetTCP] ?
| return nil, nil, false | ||
| } | ||
|
|
||
| // UpdateControllerPosition updates the controller mappings after a position change |
| uiShowModalDialog(NewModalDialogBox(&BroadcastModalDialog{Message: event.WrittenText}, p), false) | ||
| } else if event.Type == sim.StatusMessageEvent && event.ToController != "" && controlClient != nil && event.ToController == controlClient.State.UserTCP { | ||
| // Check if this is a kick notification | ||
| if strings.Contains(event.WrittenText, "You have been kicked by") { |
There was a problem hiding this comment.
I think it's a little brittle to match the text here since it's non-obvious that changing it in server/manager.go would break anything. I think it would be better and more clear to add a new KickedControllerEvent in sim/eventstream.go and to trigger on that here.
| text: "Ok", | ||
| action: func() bool { | ||
| // When the user clicks OK, show the connection dialog | ||
| uiShowConnectDialog(k.mgr, true, k.config, k.platform, k.logger) |
There was a problem hiding this comment.
Out of curiosity, is this necessary? I'd have guessed that after they were kicked the code that checks if you're connected to a sim would notice they weren't and would put this up on its own. (Then that'd be nice so you wouldn't have to carry along all of the extra members in the KickNotificationModalClient struct.)
| return ErrNoSimForControllerToken | ||
| } | ||
|
|
||
| // Any signed-in controller can kick others (no instructor/RPO restriction) |
There was a problem hiding this comment.
Is this going to cause mayhem (vs. e.g. only allowing the primary controller who is presumably the one who created the sim) to do this? I'm fine with leaving it as is and seeing how it goes, though.
|
|
||
| // Perform the kick after a short delay to allow notification delivery | ||
| go func() { | ||
| time.Sleep(500 * time.Millisecond) // Give time for notification to be processed |
There was a problem hiding this comment.
I worry that in some cases this may not be enough time and that in general it could be brittle. How about this for an alternate approach?
- Add the KickedController event type, as suggested above.
- Remove this server-side code to call signOff after a little while
- Then in cmd/vice/ui.go around line 300 where it processes all of the events, it checks for a kicked event and sees if the controller matches
UserTCP. In that case, the dialog is displayed and then you callControlClientDisconnect(), which in turn calls back to the server with aSignOffrequest. So it's kind of odd in that the user's own computer is seeing they were kicked and then asking to be kicked, but that way we know 100% that they got the message.
(Though having suggested that, there is some risk that someone could be unkickable if they disabled that in their local vice build. Maybe another alternative is to defer the kick until Sim GetStateUpdate is called by that user and we know that they will be getting the event that triggers the notification.)
|
Sorry, I've been really busy these past few weeks. I plan to work on this tomorrow |
2 participants
Resolves #246