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
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ template <class T>
requires std::copy_constructible<T> && std::default_initializable<T>
class SendableChooser : public SendableChooserBase {
wpi::StringMap<T> m_choices;
std::function<void(T)> m_listener;
std::function<void(std::string_view, T)> m_listener;
template <class U>
static U _unwrap_smart_ptr(const U& value) {
return value;
Expand Down Expand Up @@ -95,13 +95,7 @@ class SendableChooser : public SendableChooserBase {
* @return The option selected
*/
CopyType GetSelected() const {
std::string selected = m_defaultChoice;
{
std::scoped_lock lock(m_mutex);
if (m_haveSelected) {
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();

if (selected.empty()) {
return CopyType{};
} else {
Expand All @@ -113,17 +107,45 @@ class SendableChooser : public SendableChooserBase {
}
}

/**
* Returns the name of the selected option.
*
* If there is none selected, it will return the default option's name. If
* there is none selected and no default, it will return an empty string.
*
* @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.

std::scoped_lock lock(m_mutex);
if (m_haveSelected) {
return m_selected;
} else {
return m_defaultChoice;
}
}

/**
* Bind a listener that's called when the selected value changes.
* Only one listener can be bound. Calling this function will replace the
* previous listener.
* @param listener The function to call that accepts the new value
* @param listener The function to call that accepts the new name and new
* value
*/
void OnChange(std::function<void(T)> listener) {
void OnChange(std::function<void(std::string_view, T)> listener) {
std::scoped_lock lock(m_mutex);
m_listener = listener;
}

/**
* Bind a listener that's called when the selected value changes.
* Only one listener can be bound. Calling this function will replace the
* previous listener.
* @param listener The function to call that accepts the new value
*/
void OnChange(std::function<void(T)> listener) {
OnChange([listener](std::string_view val, T choice) { listener(choice); });
Braykoff marked this conversation as resolved.
Show resolved Hide resolved
}

void InitSendable(wpi::SendableBuilder& builder) override {
builder.SetSmartDashboardType("String Chooser");
builder.PublishConstInteger(kInstance, m_instance);
Expand Down Expand Up @@ -155,24 +177,24 @@ class SendableChooser : public SendableChooserBase {
}
},
nullptr);
builder.AddStringProperty(kSelected, nullptr,
[=, this](std::string_view val) {
T choice{};
std::function<void(T)> listener;
{
std::scoped_lock lock(m_mutex);
m_haveSelected = true;
m_selected = val;
if (m_previousVal != val && m_listener) {
choice = m_choices[val];
listener = m_listener;
}
m_previousVal = val;
}
if (listener) {
listener(choice);
}
});
builder.AddStringProperty(
kSelected, nullptr, [=, this](std::string_view val) {
T choice{};
std::function<void(std::string_view, T)> listener;
{
std::scoped_lock lock(m_mutex);
m_haveSelected = true;
m_selected = val;
if (m_previousVal != val && m_listener) {
choice = m_choices[val];
listener = m_listener;
}
m_previousVal = val;
}
if (listener) {
listener(val, choice);
}
});
}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ TEST_P(SendableChooserTest, ReturnsSelected) {
chooserSim.SetSelected(std::to_string(GetParam()));
frc::SmartDashboard::UpdateValues();
EXPECT_EQ(GetParam(), chooser.GetSelected());
EXPECT_EQ(std::to_string(GetParam()), chooser.GetSelectedName());
}

TEST(SendableChooserTest, DefaultIsReturnedOnNoSelect) {
Expand All @@ -44,6 +45,7 @@ TEST(SendableChooserTest, DefaultIsReturnedOnNoSelect) {
chooser.SetDefaultOption("4", 4);

EXPECT_EQ(4, chooser.GetSelected());
EXPECT_EQ("4", chooser.GetSelectedName());
}

TEST(SendableChooserTest,
Expand All @@ -55,6 +57,7 @@ TEST(SendableChooserTest,
}

EXPECT_EQ(0, chooser.GetSelected());
EXPECT_EQ("", chooser.GetSelectedName());
}

TEST(SendableChooserTest, ChangeListener) {
Expand All @@ -65,15 +68,21 @@ TEST(SendableChooserTest, ChangeListener) {
for (int i = 1; i <= 3; i++) {
chooser.AddOption(std::to_string(i), i);
}

std::string currentName = "";
int currentVal = 0;
chooser.OnChange([&](int val) { currentVal = val; });
chooser.OnChange([&](std::string_view name, int val) {
currentName = std::string(name);
currentVal = val;
});

frc::SmartDashboard::PutData("ChangeListenerChooser", &chooser);
frc::SmartDashboard::UpdateValues();
chooserSim.SetSelected("3");
frc::SmartDashboard::UpdateValues();

EXPECT_EQ(3, currentVal);
EXPECT_EQ("3", currentName);
}

INSTANTIATE_TEST_SUITE_P(SendableChooserTests, SendableChooserTest,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import java.util.Map;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.locks.ReentrantLock;
import java.util.function.BiConsumer;
import java.util.function.Consumer;

/**
Expand Down Expand Up @@ -49,7 +50,7 @@ public class SendableChooser<V> implements Sendable, AutoCloseable {
private String m_defaultChoice = "";
private final int m_instance;
private String m_previousVal;
private Consumer<V> m_listener;
private BiConsumer<String, V> m_listener;
private static final AtomicInteger s_instances = new AtomicInteger();

/** Instantiates a {@link SendableChooser}. */
Expand Down Expand Up @@ -97,12 +98,22 @@ public void setDefaultOption(String name, V object) {
* @return the option selected
*/
public V getSelected() {
return m_map.get(getSelectedName());
}

/**
* Returns the name of the selected option. If there is none selected, it will return the default.
* If there is none selected and no default, it will return an empty String.
*
* @return the name of the option selected
*/
public String getSelectedName() {
m_mutex.lock();
try {
if (m_selected != null) {
return m_map.get(m_selected);
return m_selected;
} else {
return m_map.get(m_defaultChoice);
return m_defaultChoice;
}
} finally {
m_mutex.unlock();
Expand All @@ -113,15 +124,25 @@ public V getSelected() {
* Bind a listener that's called when the selected value changes. Only one listener can be bound.
* Calling this function will replace the previous listener.
*
* @param listener The function to call that accepts the new value
* @param listener The function to call that accepts the new name and new value
*/
public void onChange(Consumer<V> listener) {
public void onChange(BiConsumer<String, V> listener) {
requireNonNullParam(listener, "listener", "onChange");
m_mutex.lock();
m_listener = listener;
m_mutex.unlock();
}

/**
* Bind a listener that's called when the selected value changes. Only one listener can be bound.
* Calling this function will replace the previous listener.
*
* @param listener The function to call that accepts the new value
*/
public void onChange(Consumer<V> listener) {
onChange((String value, V choice) -> listener.accept(choice));
}

private String m_selected;
private final ReentrantLock m_mutex = new ReentrantLock();

Expand Down Expand Up @@ -151,7 +172,7 @@ public void initSendable(SendableBuilder builder) {
null,
val -> {
V choice;
Consumer<V> listener;
BiConsumer<String, V> listener;
m_mutex.lock();
try {
m_selected = val;
Expand All @@ -167,7 +188,7 @@ public void initSendable(SendableBuilder builder) {
m_mutex.unlock();
}
if (listener != null) {
listener.accept(choice);
listener.accept(val, choice);
}
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import edu.wpi.first.networktables.NetworkTableInstance;
import edu.wpi.first.wpilibj.simulation.SendableChooserSim;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -42,6 +43,7 @@ void returnsSelected(int toSelect) {
chooserSim.setSelected(String.valueOf(toSelect));
SmartDashboard.updateValues();
assertEquals(toSelect, chooser.getSelected());
assertEquals(String.valueOf(toSelect), chooser.getSelectedName());
}
}

Expand All @@ -54,6 +56,7 @@ void defaultIsReturnedOnNoSelect() {
chooser.setDefaultOption(String.valueOf(0), 0);

assertEquals(0, chooser.getSelected());
assertEquals(String.valueOf(0), chooser.getSelectedName());
}
}

Expand All @@ -65,6 +68,7 @@ void nullIsReturnedOnNoSelectAndNoDefault() {
}

assertNull(chooser.getSelected());
assertEquals("", chooser.getSelectedName());
}
}

Expand All @@ -75,14 +79,21 @@ void testChangeListener() {
for (int i = 1; i <= 3; i++) {
chooser.addOption(String.valueOf(i), i);
}

AtomicReference<String> currentName = new AtomicReference<>("");
AtomicInteger currentVal = new AtomicInteger();
chooser.onChange(currentVal::set);
chooser.onChange(
(String name, Integer value) -> {
currentName.set(name);
currentVal.set(value);
});

SmartDashboard.putData("changeListenerChooser", chooser);
SmartDashboard.updateValues();
chooserSim.setSelected("3");
SmartDashboard.updateValues();
assertEquals(3, currentVal.get());
assertEquals("3", currentName.get());
}
}

Expand Down
Loading