-
-
Notifications
You must be signed in to change notification settings - Fork 489
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
feat(adb): add client flavors and autolaunch #2515
base: master
Are you sure you want to change the base?
Conversation
@@ -24,7 +25,7 @@ impl WiredConnection { | |||
Ok(Self { adb_path }) | |||
} | |||
|
|||
pub fn setup(&self, control_port: u16, stream_port: u16) -> Result<WiredConnectionStatus> { | |||
pub fn setup(&self, control_port: u16, settings: &Settings) -> Result<WiredConnectionStatus> { |
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 would pass the minimum subset of settings as parameter. This would be ConnectionConfig
#[schema(strings( | ||
help = r#"Wether ALVR should try to automatically launch the client when establishing a wired connection."# | ||
))] | ||
pub client_autolaunch: bool, | ||
|
||
#[schema(strings( | ||
help = r#"Which type of client should ALVR look for when establishing a wired connection."# | ||
))] | ||
pub client_flavor: ClientFlavor, |
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 would rename these to wired_client_autolaunch
and wired_client_version_check
. I would also switch the order of these two settings. The reason is wired_client_autolaunch
requires wired_client_version_check
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.
version_check makes no sense tho, this isn't about a the v version, which is probably going to confuse people
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.
Right. it's that "flavor" may sound a bit funny. I don't have better ideas so let's use wired_client_flavor_check
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.
wired_client_type and maybe adjust the help string to "Which type of release of the client ..."
let process_name = match &settings.connection.client_flavor { | ||
ClientFlavor::Store => if alvr_common::is_stable() { | ||
"alvr.client" | ||
} else { | ||
"alvr.client.dev" | ||
} | ||
.to_owned(), | ||
ClientFlavor::Github => if alvr_common::is_stable() { | ||
"alvr.client.stable" | ||
} else { | ||
"alvr.client.dev" | ||
} | ||
.to_owned(), | ||
ClientFlavor::Custom(name) => name.clone(), | ||
}; |
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.
This is missing the fallbacks.
Store: store -> gh
GitHub: gh -> store
Custom: custom -> store -> gh
Skip store if not stable.
You can implement this by storing a slice to process_names
. At this point it would be better to extract the package names to string constants.
No description provided.