Fix and add tests for sort order

wpilibc/src/main/native/cpp/Alert.cpp

@@ -28,7 +28,13 @@ class Alert::PublishedAlert {
       : timestamp{timestamp}, text{text} {}
   uint64_t timestamp;
   std::string text;
-  auto operator<=>(const PublishedAlert&) const = default;
+  auto operator<=>(const PublishedAlert& other) const {
+    if (timestamp != other.timestamp) {
+      return other.timestamp <=> timestamp;
+    } else {
+      return text <=> other.text;
+    }
+  }
 };
 
 class Alert::SendableAlerts : public nt::NTSendable,

wpilibc/src/main/native/include/frc/Alert.h

@@ -16,7 +16,7 @@ namespace frc {
  * of kError, kWarning, or kInfo to denote urgency. See Alert::AlertType for
  * suggested usage of each type. Alerts can be displayed on supported
  * dashboards, and are shown in a priority order based on type and recency of
- * activation.
+ * activation, with newly activated alerts first.
  *
  * Alerts should be created once and stored persistently, then updated to
  * "active" or "inactive" as necessary. Set(bool) can be safely called
@@ -86,6 +86,9 @@ class Alert {
   Alert(Alert&&);
   Alert& operator=(Alert&&);
 
+  Alert(const Alert&) = default;
+  Alert& operator=(const Alert&) = default;
+
   ~Alert();
 
   /**

wpilibc/src/test/native/cpp/AlertTest.cpp

@@ -2,20 +2,22 @@
 // Open Source Software; you can modify and/or share it under the terms of
 // the WPILib BSD license file in the root directory of this project.
 
-#include <frc/Alert.h>
-#include <frc/smartdashboard/SmartDashboard.h>
-
 #include <algorithm>
 #include <chrono>
 #include <map>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include <fmt/format.h>
 #include <gtest/gtest.h>
 #include <networktables/NetworkTableInstance.h>
 #include <networktables/StringArrayTopic.h>
 
+#include "frc/Alert.h"
+#include "frc/simulation/SimHooks.h"
+#include "frc/smartdashboard/SmartDashboard.h"
+
 using namespace frc;
 using enum Alert::AlertType;
 class AlertsTest : public ::testing::Test {
@@ -40,9 +42,13 @@ class AlertsTest : public ::testing::Test {
     return Alert(GetGroupName(), std::forward<Args>(args)...);
   }
 
-  bool IsAlertActive(std::string_view text, Alert::AlertType type) {
+  std::vector<std::string> GetActiveAlerts(Alert::AlertType type) {
     Update();
-    auto activeAlerts = GetSubscriberForType(type).Get();
+    return GetSubscriberForType(type).Get();
+  }
+
+  bool IsAlertActive(std::string_view text, Alert::AlertType type) {
+    auto activeAlerts = GetActiveAlerts(type);
     return std::find(activeAlerts.begin(), activeAlerts.end(), text) !=
            activeAlerts.end();
   }
@@ -71,6 +77,9 @@ class AlertsTest : public ::testing::Test {
   }
 };
 
+#define EXPECT_STATE(type, ...) \
+  EXPECT_EQ(GetActiveAlerts(type), (std::vector<std::string>{__VA_ARGS__}))
+
 TEST_F(AlertsTest, SetUnset) {
   auto one = MakeAlert("one", kError);
   auto two = MakeAlert("two", kInfo);
@@ -88,6 +97,24 @@ TEST_F(AlertsTest, SetUnset) {
   EXPECT_TRUE(IsAlertActive("two", kInfo));
 }
 
+TEST_F(AlertsTest, SetIsIdempotent) {
+  auto a = MakeAlert("A", kInfo);
+  auto b = MakeAlert("B", kInfo);
+  auto c = MakeAlert("C", kInfo);
+  a.Set(true);
+
+  b.Set(true);
+  c.Set(true);
+
+  const auto startState = GetActiveAlerts(kInfo);
+
+  b.Set(true);
+  EXPECT_STATE(kInfo, startState);
+
+  a.Set(true);
+  EXPECT_STATE(kInfo, startState);
+}
+
 TEST_F(AlertsTest, DestructorUnsetsAlert) {
   {
     auto alert = MakeAlert("alert", kWarning);
@@ -122,6 +149,28 @@ TEST_F(AlertsTest, SetTextWhileSet) {
   EXPECT_TRUE(IsAlertActive("AFTER", kInfo));
 }
 
+TEST_F(AlertsTest, SetTextDoesNotAffectFirstOrderSort) {
+  frc::sim::PauseTiming();
+
+  auto a = MakeAlert("A", kError);
+  auto b = MakeAlert("B", kError);
+  auto c = MakeAlert("C", kError);
+
+  a.Set(true);
+  frc::sim::StepTiming(1_s);
+  b.Set(true);
+  frc::sim::StepTiming(1_s);
+  c.Set(true);
+
+  auto expectedEndState = GetActiveAlerts(kError);
+  std::replace(expectedEndState.begin(), expectedEndState.end(),
+               std::string("B"), std::string("AFTER"));
+  b.SetText("AFTER");
+
+  EXPECT_STATE(kError, expectedEndState);
+  frc::sim::ResumeTiming();
+}
+
 TEST_F(AlertsTest, MoveAssign) {
   auto outer = MakeAlert("outer", kInfo);
   outer.Set(true);
@@ -150,3 +199,44 @@ TEST_F(AlertsTest, MoveConstruct) {
   b.Set(true);
   EXPECT_TRUE(IsAlertActive("A", kInfo));
 }
+
+TEST_F(AlertsTest, SortOrder) {
+  frc::sim::PauseTiming();
+  auto a = MakeAlert("A", kInfo);
+  auto b = MakeAlert("B", kInfo);
+  auto c = MakeAlert("C", kInfo);
+  a.Set(true);
+  EXPECT_STATE(kInfo, "A");
+  frc::sim::StepTiming(1_s);
+  b.Set(true);
+  EXPECT_STATE(kInfo, "B", "A");
+  frc::sim::StepTiming(1_s);
+  c.Set(true);
+  EXPECT_STATE(kInfo, "C", "B", "A");
+
+  frc::sim::StepTiming(1_s);
+  c.Set(false);
+  EXPECT_STATE(kInfo, "B", "A");
+
+  frc::sim::StepTiming(1_s);
+  c.Set(true);
+  EXPECT_STATE(kInfo, "C", "B", "A");
+
+  frc::sim::StepTiming(1_s);
+  a.Set(false);
+  EXPECT_STATE(kInfo, "C", "B");
+
+  frc::sim::StepTiming(1_s);
+  b.Set(false);
+  EXPECT_STATE(kInfo, "C");
+
+  frc::sim::StepTiming(1_s);
+  b.Set(true);
+  EXPECT_STATE(kInfo, "B", "C");
+
+  frc::sim::StepTiming(1_s);
+  a.Set(true);
+  EXPECT_STATE(kInfo, "A", "B", "C");
+
+  frc::sim::ResumeTiming();
+}

wpilibj/src/main/java/edu/wpi/first/wpilibj/Alert.java

@@ -18,7 +18,7 @@
  * {@code kWarning}, or {@code kInfo} to denote urgency. See {@link
  * edu.wpi.first.wpilibj.Alert.AlertType AlertType} for suggested usage of each type. Alerts can be
  * displayed on supported dashboards, and are shown in a priority order based on type and recency of
- * activation.
+ * activation, with newly activated alerts first.
  *
  * <p>Alerts should be created once and stored persistently, then updated to "active" or "inactive"
  * as necessary. {@link #set(boolean)} can be safely called periodically.
@@ -171,6 +171,7 @@ public AlertType getType() {
   private record PublishedAlert(long timestamp, String text) implements Comparable<PublishedAlert> {
     private static final Comparator<PublishedAlert> comparator =
         Comparator.comparingLong((PublishedAlert alert) -> alert.timestamp())
+            .reversed()
             .thenComparing(Comparator.comparing((PublishedAlert alert) -> alert.text()));
 
     @Override
@@ -180,7 +181,6 @@ public int compareTo(PublishedAlert o) {
   }
 
   private static final class SendableAlerts implements Sendable {
-    // TODO: I think we could use WeakReference here to automatically remove dangling alerts
     private final Map<AlertType, Set<PublishedAlert>> m_alerts = new HashMap<>();
 
     /**

wpilibj/src/test/java/edu/wpi/first/wpilibj/AlertTest.java

@@ -11,12 +11,16 @@
 import edu.wpi.first.networktables.NetworkTableInstance;
 import edu.wpi.first.networktables.StringArraySubscriber;
 import edu.wpi.first.wpilibj.Alert.AlertType;
+import edu.wpi.first.wpilibj.simulation.SimHooks;
 import edu.wpi.first.wpilibj.smartdashboard.SmartDashboard;
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.List;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.parallel.ResourceLock;
 
 class AlertTest {
   String m_groupName;
@@ -54,6 +58,7 @@ private StringArraySubscriber getSubscriberForType(Alert.AlertType type) {
   }
 
   private String[] getActiveAlerts(AlertType type) {
+    update();
     try (var sub = getSubscriberForType(type)) {
       return sub.get();
     }
@@ -64,10 +69,13 @@ private void update() {
   }
 
   private boolean isAlertActive(String text, Alert.AlertType type) {
-    update();
     return Arrays.asList(getActiveAlerts(type)).contains(text);
   }
 
+  private void assertState(Alert.AlertType type, List<String> state) {
+    assertEquals(state, Arrays.asList(getActiveAlerts(type)));
+  }
+
   private Alert makeAlert(String text, Alert.AlertType type) {
     return new Alert(m_groupName, text, type);
   }
@@ -91,6 +99,25 @@ void setUnset() {
     }
   }
 
+  @Test
+  void setIsIdempotent() {
+    try (var a = makeAlert("A", AlertType.kInfo);
+        var b = makeAlert("B", AlertType.kInfo);
+        var c = makeAlert("C", AlertType.kInfo)) {
+      a.set(true);
+      b.set(true);
+      c.set(true);
+
+      var startState = List.of(getActiveAlerts(AlertType.kInfo));
+
+      b.set(true);
+      assertState(AlertType.kInfo, startState);
+
+      a.set(true);
+      assertState(AlertType.kInfo, startState);
+    }
+  }
+
   @Test
   void closeUnsetsAlert() {
     try (var alert = makeAlert("alert", AlertType.kWarning)) {
@@ -128,4 +155,71 @@ void setTextWhileSet() {
       assertTrue(isAlertActive("AFTER", AlertType.kInfo));
     }
   }
+
+  @ResourceLock("timing")
+  @Test
+  void setTextDoesNotAffectFirstOrderSort() {
+    SimHooks.pauseTiming();
+    try (var a = makeAlert("A", AlertType.kInfo);
+        var b = makeAlert("B", AlertType.kInfo);
+        var c = makeAlert("C", AlertType.kInfo)) {
+      a.set(true);
+      SimHooks.stepTiming(1);
+      b.set(true);
+      SimHooks.stepTiming(1);
+      c.set(true);
+
+      var expectedEndState = new ArrayList<>(List.of(getActiveAlerts(AlertType.kInfo)));
+      expectedEndState.replaceAll(s -> "B".equals(s) ? "AFTER" : s);
+      b.setText("AFTER");
+
+      assertState(AlertType.kInfo, expectedEndState);
+    } finally {
+      SimHooks.resumeTiming();
+    }
+  }
+
+  @ResourceLock("timing")
+  @Test
+  void sortOrder() {
+    SimHooks.pauseTiming();
+    try (var a = makeAlert("A", AlertType.kInfo);
+        var b = makeAlert("B", AlertType.kInfo);
+        var c = makeAlert("C", AlertType.kInfo)) {
+      a.set(true);
+      assertState(AlertType.kInfo, List.of("A"));
+      SimHooks.stepTiming(1);
+      b.set(true);
+      assertState(AlertType.kInfo, List.of("B", "A"));
+      SimHooks.stepTiming(1);
+      c.set(true);
+      assertState(AlertType.kInfo, List.of("C", "B", "A"));
+
+      SimHooks.stepTiming(1);
+      c.set(false);
+      assertState(AlertType.kInfo, List.of("B", "A"));
+
+      SimHooks.stepTiming(1);
+      c.set(true);
+      assertState(AlertType.kInfo, List.of("C", "B", "A"));
+
+      SimHooks.stepTiming(1);
+      a.set(false);
+      assertState(AlertType.kInfo, List.of("C", "B"));
+
+      SimHooks.stepTiming(1);
+      b.set(false);
+      assertState(AlertType.kInfo, List.of("C"));
+
+      SimHooks.stepTiming(1);
+      b.set(true);
+      assertState(AlertType.kInfo, List.of("B", "C"));
+
+      SimHooks.stepTiming(1);
+      a.set(true);
+      assertState(AlertType.kInfo, List.of("A", "B", "C"));
+    } finally {
+      SimHooks.resumeTiming();
+    }
+  }
 }