-
Notifications
You must be signed in to change notification settings - Fork 92
add --auto-subscribe cli option to enable auto-subscribe to published tracks in a room #566
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.
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
Conversation
&cli.BoolFlag{ | ||
Name: "auto-subscribe", | ||
Usage: "Automatically subscribe to published tracks. Default is false.", | ||
Value: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm hesitant to change the default behavior in this PR. maybe best as true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seconded. Also, when Value
is set on a flag, the CLI will display (default: $value)
so you can remove it from the usage string.
EDIT: and then I read the note below, false
makes sense.
cmd/lk/room.go
Outdated
&cli.BoolFlag{ | ||
Name: "auto-subscribe", | ||
Usage: "Automatically subscribe to published tracks. Default is true.", | ||
Value: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should probably be false by default. the CLI client cannot really consume any tracks
This reverts commit 82aa183.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor changes based on how we normally do CLI flags!
&cli.BoolFlag{ | ||
Name: "auto-subscribe", | ||
Usage: "Automatically subscribe to published tracks. Default is false.", | ||
Value: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seconded. Also, when Value
is set on a flag, the CLI will display (default: $value)
so you can remove it from the usage string.
EDIT: and then I read the note below, false
makes sense.
cmd/lk/utils.go
Outdated
func extractFlagAsBool(c *cli.Command, flag string) (bool, error) { | ||
if c.IsSet(flag) { | ||
return c.Bool(flag), nil | ||
} | ||
|
||
return false, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see anything that could possibly error here, so it doesn't need to return one
cmd/lk/room.go
Outdated
autoSubscribe, err := extractFlagAsBool(cmd, "auto-subscribe") | ||
if err != nil { | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just read cmd.Bool("auto-subscribe")
directly, since we are setting a default value for it.
version.go
Outdated
@@ -15,5 +15,5 @@ | |||
package livekitcli | |||
|
|||
const ( | |||
Version = "2.4.5" | |||
Version = "2.4.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need a rebase, we're at 2.4.6
already
When using LK CLI to directly publish tracks into a room, it defaults to also auto-subscribing to other participant's tracks. This is undesirable as most use cases of the CLI does not need to receive any tracks and will waste bandwidth. This adds a
--auto-subscribe
parameter to the CLI when using the join room functionality to explicitly enable auto-subscribing to published tracks.