Skip to content

Add support for retrieving and displaying Switch labels #411

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

Merged
merged 6 commits into from
Aug 11, 2025

Conversation

Joyceku1020
Copy link
Contributor

  • Added getLabels() to Switch interface and SwitchClient

  • Updated ViamSwitchWidget to display labels if available

@Joyceku1020 Joyceku1020 requested a review from a team as a code owner August 1, 2025 14:26
@CLAassistant
Copy link

CLAassistant commented Aug 1, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@njooma njooma left a comment

Choose a reason for hiding this comment

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

I don't think we should have a separate getLabels method, but instead return the labels with the getNumberOfPositions

/// ```
///
/// For more information, see [Switch component](https://docs.viam.com/dev/reference/apis/components/servo/#getnumberofpositions).
Future<List<String>> getLabels({Map<String, dynamic>? extra});
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be folded into getNumberOfPositions because you're likely not displaying labels without getting number of positions first, so it doesn't make sense to have 2 distinct calls for it (IMO)

@@ -30,12 +30,21 @@ abstract class Switch extends Resource {
/// Get the number of available positions (int) of the [Switch].
///
/// ```
/// await myServo.getNumberOfPositions();
/// await mySwitch.getNumberOfPositions();
/// ```
///
/// For more information, see [Switch component](https://docs.viam.com/dev/reference/apis/components/servo/#getnumberofpositions).
Future<int> getNumberOfPositions({Map<String, dynamic>? extra});
Copy link
Member

Choose a reason for hiding this comment

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

So return both int and List<String> here (I don't think you can return tuples in Flutter, so you might want to alias GetNumberOfPositionsResponse to PositionsAndLabels or something similar and return that)

Comment on lines 82 to 86
test('getLabels should return the expected labels', () async {
final labels = await nswitch.getLabels();
expect(labels, nswitch.labels);
});

Copy link
Member

@jckras jckras Aug 4, 2025

Choose a reason for hiding this comment

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

great testing!

@Joyceku1020 Joyceku1020 requested review from njooma and jckras August 4, 2025 14:13
Comment on lines 9 to 15
class PositionsInfo {
final int numberOfPositions;
final List<String>? labels;

PositionsInfo({required this.numberOfPositions, this.labels});
}

Copy link
Member

Choose a reason for hiding this comment

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

Because this class mirrors the attributes in GetNumberOfPositionsResponse, it's probably easier to typealias (see camera example)

@Joyceku1020 Joyceku1020 requested a review from njooma August 8, 2025 18:41
Copy link
Member

@njooma njooma left a comment

Choose a reason for hiding this comment

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

Ok last thing!!!

Comment on lines +70 to +80
Future<void> _getLabels() async {
try {
final positionsInfo = await widget.nswitch.getNumberOfPositionsWithLabels();
if (mounted) {
setState(() {
labels = positionsInfo.labels;
});
}
} catch (e) {
error = e as Error;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think you can delete this method, and set the labels state when you get it above in the _getNumberOfPositions method

@njooma njooma merged commit 809826e into viamrobotics:main Aug 11, 2025
4 checks passed
@jckras
Copy link
Member

jckras commented Aug 11, 2025

thanks for completing this @njooma

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants