Skip to content
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

Add methods to get name of SendableChooser's selected value #7580

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Braykoff
Copy link
Contributor

In some cases, such as logging, it can be useful to get the name of a SendableChooser's currently selected option. This adds:

  • getSelectedName()/GetSelectedName(), to get the name of the currently selected option
  • onChange(BiConsumer<String, V>)/OnChange(std::function<void(std::string_view, T)> listener) to get the name when the currently selected option changes. The original onChange methods are kept for backward compatibility.

@Braykoff Braykoff requested a review from a team as a code owner December 23, 2024 03:54
@Braykoff
Copy link
Contributor Author

/format

@Gold856
Copy link
Contributor

Gold856 commented Dec 23, 2024

/format was removed. You'll have to format manually.

@Braykoff Braykoff changed the title Add methods to get name of SendableChooser's selected value. Add methods to get name of SendableChooser's selected value Dec 23, 2024
Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

Great work! I'm really hopeful that this can make it in before kickoff. Just a few things:

  • You can change the first 7 lines of GetSelected() (currently 98-104) to just std::string selected = GetSelectedName(); to match Java.
  • Java's testChangeListener() doesn't have the changes done to C++'s ChangeListener test.

@Braykoff
Copy link
Contributor Author

It seems an unrelated test has failed in ubsan:

[----------] Global test environment tear-down
[==========] 401 tests from 68 test suites ran. (7832 ms total)
[  PASSED  ] 400 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] DifferentialDrivetrainSimTest.Convergence

 1 FAILED TEST

*
* @return The name of the option selected
*/
std::string_view GetSelectedName() const {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::string_view GetSelectedName() const {
std::string GetSelectedName() const {

It's not safe to return a string_view here. It needs to return a copy (a std::string) because the access is mutex-protected, so (theoretically) another thread could come in and modify m_selected before the string_view is accessed.

selected = m_selected;
}
}
std::string_view selected = GetSelectedName();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::string_view selected = GetSelectedName();
std::string selected = GetSelectedName();

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

Successfully merging this pull request may close these issues.

4 participants