Suspend property bindings when their screens aren't visible

src/lua/include/property.hpp

@@ -33,17 +33,33 @@ class Property {
  public:
   Property() : Property(std::monostate{}) {}
   Property(const LuaValue&);
-  Property(const LuaValue&, std::function<bool(const LuaValue&)>);
+  Property(const LuaValue&, std::function<bool(const LuaValue&)> filter);
 
-  auto Get() -> const LuaValue& { return *value_; }
+  auto get() -> const LuaValue& { return *value_; }
 
-  auto IsTwoWay() -> bool { return cb_.has_value(); }
+  /*
+   * Assigns a new value to this property, bypassing the filter fn. All
+   * bindings will be marked as dirty, and if active, will be reapplied.
+   */
+  auto setDirect(const LuaValue&) -> void;
+  /*
+   * Invokes the filter fn, and if successful, assigns the new value to this
+   * property. All bindings will be marked as dirty, and if active, will be
+   * reapplied.
+   */
+  auto set(const LuaValue&) -> bool;
 
-  auto PushValue(lua_State& s) -> int;
-  auto PopValue(lua_State& s) -> bool;
-  auto Update(const LuaValue& new_val) -> void;
+  /* Returns whether or not this Property can be written from Lua. */
+  auto isTwoWay() -> bool { return cb_.has_value(); }
 
-  auto AddLuaBinding(lua_State*, int ref) -> void;
+  auto pushValue(lua_State& s) -> int;
+  auto popValue(lua_State& s) -> bool;
+
+  /* Reapplies all active, dirty bindings associated with this Property. */
+  auto reapplyAll() -> void;
+
+  auto addLuaBinding(lua_State*, int ref) -> void;
+  auto applySingle(lua_State*, int ref, bool mark_dirty) -> bool;
 
  private:
   std::unique_ptr<LuaValue> value_;
@@ -51,6 +67,28 @@ class Property {
   std::pmr::vector<std::pair<lua_State*, int>> bindings_;
 };
 
+/*
+ * Container for a Lua function that should be invoked whenever a Property's
+ * value changes, as well as some extra accounting metadata.
+ */
+struct Binding {
+  /* Checks the value at idx is a Binding, returning a pointer to it if so. */
+  static auto get(lua_State*, int idx) -> Binding*;
+  /*
+   * If the value at idx is a dirty, active Binding, applies the current value
+   * from its Property. Returns false if the binding was active and dirty, but
+   * invoking the Lua callback failed.
+   */
+  static auto apply(lua_State*, int idx) -> bool;
+
+  Property* property;
+  bool active;
+  bool dirty;
+};
+
+static_assert(std::is_trivially_destructible<Binding>());
+static_assert(std::is_trivially_copy_assignable<Binding>());
+
 class PropertyBindings {
  public:
   PropertyBindings();

src/lua/property.cpp

@@ -29,9 +29,28 @@ namespace lua {
 
 static const char kPropertyMetatable[] = "property";
 static const char kFunctionMetatable[] = "c_func";
+static const char kBindingMetatable[] = "binding";
 static const char kBindingsTable[] = "bindings";
 static const char kBinderKey[] = "binder";
 
+auto Binding::get(lua_State* L, int idx) -> Binding* {
+  return reinterpret_cast<Binding*>(luaL_testudata(L, idx, kBindingMetatable));
+}
+
+auto Binding::apply(lua_State* L, int idx) -> bool {
+  Binding* b = get(L, idx);
+  if (b->dirty && b->active) {
+    b->dirty = false;
+    // The binding needs to be reapplied. Push the Lua callback, then its arg.
+    lua_getiuservalue(L, idx, 1);
+    b->property->pushValue(*L);
+
+    // Invoke the callback.
+    return CallProtected(L, 1, 0) == LUA_OK;
+  }
+  return true;
+}
+
 static auto check_property(lua_State* state) -> Property* {
   void* data = luaL_checkudata(state, 1, kPropertyMetatable);
   luaL_argcheck(state, data != NULL, 1, "`property` expected");
@@ -40,14 +59,14 @@ static auto check_property(lua_State* state) -> Property* {
 
 static auto property_get(lua_State* state) -> int {
   Property* p = check_property(state);
-  p->PushValue(*state);
+  p->pushValue(*state);
   return 1;
 }
 
 static auto property_set(lua_State* state) -> int {
   Property* p = check_property(state);
-  luaL_argcheck(state, p->IsTwoWay(), 1, "property is read-only");
-  bool valid = p->PopValue(*state);
+  luaL_argcheck(state, p->isTwoWay(), 1, "property is read-only");
+  bool valid = p->popValue(*state);
   lua_pushboolean(state, valid);
   return 1;
 }
@@ -56,35 +75,40 @@ static auto property_bind(lua_State* state) -> int {
   Property* p = check_property(state);
   luaL_checktype(state, 2, LUA_TFUNCTION);
 
-  // Copy the function, as we need to invoke it then store our reference.
-  lua_pushvalue(state, 2);
-  // ...and another copy, since we return the original closure.
-  lua_pushvalue(state, 2);
-
-  p->PushValue(*state);
-  CallProtected(state, 1, 0);  // Invoke the initial binding.
-
+  // Fetch the table of live bindings.
   lua_pushstring(state, kBindingsTable);
   lua_gettable(state, LUA_REGISTRYINDEX);  // REGISTRY[kBindingsTable]
-  lua_insert(state, -2);          // Move bindings to the bottom, with fn above.
-  int ref = luaL_ref(state, -2);  // bindings[ref] = fn
 
-  p->AddLuaBinding(state, ref);
+  // Create the userdata holding the new binding's metadata.
+  Binding* binding =
+      reinterpret_cast<Binding*>(lua_newuserdatauv(state, sizeof(Binding), 1));
+  *binding = Binding{.property = p, .active = true, .dirty = true};
+  luaL_setmetatable(state, kBindingMetatable);
+
+  // Associate the callback function with the new binding.
+  lua_pushvalue(state, 2);
+  lua_setiuservalue(state, -2, 1);
+
+  // Put a reference to the binding into the bindings table, so that we can
+  // look it up later.
+  lua_pushvalue(state, -1);
+  int binding_ref = luaL_ref(state, 3);
 
-  // Pop the bindings table, leaving one of the copies of the callback fn at
-  // the top of the stack.
-  lua_pop(state, 1);
+  // Tell the property about the new binding. This was also perform the initial
+  // bind.
+  p->addLuaBinding(state, binding_ref);
 
+  // Return the only remaining strong reference to the new Binding.
   return 1;
 }
 
 static auto property_tostring(lua_State* state) -> int {
   Property* p = check_property(state);
-  p->PushValue(*state);
+  p->pushValue(*state);
 
   std::stringstream str{};
   str << "property { " << luaL_tolstring(state, -1, NULL);
-  if (!p->IsTwoWay()) {
+  if (!p->isTwoWay()) {
     str << ", read-only";
   }
   str << " }";
@@ -140,6 +164,11 @@ auto PropertyBindings::install(lua_State* L) -> void {
   // We've finished setting up the metatable, so pop it.
   lua_pop(L, 1);
 
+  // Create the metatable responsible for each Binding. This metatable is empty
+  // as it's only used for identification.
+  luaL_newmetatable(L, kBindingMetatable);
+  lua_pop(L, 1);
+
   // Create a weak table in the registry to hold live bindings.
   lua_pushstring(L, kBindingsTable);
   lua_newtable(L);  // bindings = {}
@@ -198,6 +227,19 @@ Property::Property(const LuaValue& val,
       cb_(cb),
       bindings_(&memory::kSpiRamResource) {}
 
+auto Property::setDirect(const LuaValue& val) -> void {
+  *value_ = val;
+  reapplyAll();
+}
+
+auto Property::set(const LuaValue& val) -> bool {
+  if (cb_ && !std::invoke(*cb_, val)) {
+    return false;
+  }
+  setDirect(val);
+  return true;
+}
+
 static auto pushTagValue(lua_State* L, const database::TagValue& val) -> void {
   std::visit(
       [&](auto&& arg) {
@@ -276,7 +318,7 @@ static void pushDevice(lua_State* L, const drivers::bluetooth::Device& dev) {
   lua_rawset(L, -3);
 }
 
-auto Property::PushValue(lua_State& s) -> int {
+auto Property::pushValue(lua_State& s) -> int {
   std::visit(
       [&](auto&& arg) {
         using T = std::decay_t<decltype(arg)>;
@@ -336,7 +378,7 @@ auto popRichType(lua_State* L) -> LuaValue {
   return std::monostate{};
 }
 
-auto Property::PopValue(lua_State& s) -> bool {
+auto Property::popValue(lua_State& s) -> bool {
   LuaValue new_val;
   switch (lua_type(&s, 2)) {
     case LUA_TNIL:
@@ -366,40 +408,53 @@ auto Property::PopValue(lua_State& s) -> bool {
       }
   }
 
-  if (cb_ && std::invoke(*cb_, new_val)) {
-    Update(new_val);
-    return true;
-  }
-  return false;
+  return set(new_val);
 }
 
-auto Property::Update(const LuaValue& v) -> void {
-  *value_ = v;
-
+auto Property::reapplyAll() -> void {
   for (int i = bindings_.size() - 1; i >= 0; i--) {
     auto& b = bindings_[i];
+    if (!applySingle(b.first, b.second, true)) {
+      // Remove the binding if we weren't able to apply it. This is usually due
+      // to the binding getting GC'd.
+      bindings_.erase(bindings_.begin() + i);
+    }
+  }
+}
 
-    int top = lua_gettop(b.first);
+auto Property::applySingle(lua_State* L, int ref, bool mark_dirty) -> bool {
+  int top = lua_gettop(L);
 
-    lua_pushstring(b.first, kBindingsTable);
-    lua_gettable(b.first, LUA_REGISTRYINDEX);       // REGISTRY[kBindingsTable]
-    int type = lua_rawgeti(b.first, -1, b.second);  // push bindings[i]
+  // Push the table of bindings.
+  lua_pushstring(L, kBindingsTable);
+  lua_gettable(L, LUA_REGISTRYINDEX);
 
-    // Has closure has been GCed?
-    if (type == LUA_TNIL) {
-      // Remove the binding.
-      bindings_.erase(bindings_.begin() + i);
-    } else {
-      PushValue(*b.first);           // push the argument
-      CallProtected(b.first, 1, 0);  // invoke the closure
-    }
+  // Resolve the reference.
+  int type = lua_rawgeti(L, -1, ref);
+  if (type == LUA_TNIL) {
+    lua_settop(L, top);
+    return false;
+  }
+
+  // Defensively check that the ref was actually for a Binding.
+  Binding* b = Binding::get(L, -1);
+  if (!b) {
+    lua_settop(L, top);
+    return false;
+  }
 
-    lua_settop(b.first, top);  // clean up after ourselves
+  if (mark_dirty) {
+    b->dirty = true;
   }
+
+  bool ret = Binding::apply(L, -1);
+  lua_settop(L, top);
+  return ret;
 }
 
-auto Property::AddLuaBinding(lua_State* state, int ref) -> void {
+auto Property::addLuaBinding(lua_State* state, int ref) -> void {
   bindings_.push_back({state, ref});
+  applySingle(state, ref, true);
 }
 
 }  // namespace lua

src/ui/include/screen_lua.hpp

@@ -8,6 +8,7 @@
 
 #include "lua.hpp"
 
+#include "property.hpp"
 #include "screen.hpp"
 
 namespace ui {
@@ -26,6 +27,11 @@ class Lua : public Screen {
   auto SetObjRef(lua_State*) -> void;
 
  private:
+  /* Invokes a method on this screen's Lua counterpart. */
+  auto callMethod(std::string name) -> void;
+  /* Applies fn to each binding in this screen's `bindings` field. */
+  auto forEachBinding(std::function<void(lua::Binding*)> fn) -> void;
+
   lua_State* s_;
   std::optional<int> obj_ref_;
 };

src/ui/screen_lua.cpp

@@ -9,6 +9,7 @@
 #include "core/lv_obj_tree.h"
 #include "lua.h"
 #include "lua.hpp"
+#include "property.hpp"
 #include "themes.hpp"
 
 #include "lua_thread.hpp"
@@ -28,28 +29,46 @@ Lua::~Lua() {
 }
 
 auto Lua::onShown() -> void {
+  callMethod("onShown");
+  forEachBinding([&](lua::Binding* b) { b->active = true; });
+}
+
+auto Lua::onHidden() -> void {
+  callMethod("onHidden");
+  forEachBinding([&](lua::Binding* b) { b->active = false; });
+}
+
+auto Lua::canPop() -> bool {
   if (!s_ || !obj_ref_) {
-    return;
+    return true;
   }
   lua_rawgeti(s_, LUA_REGISTRYINDEX, *obj_ref_);
-  lua_pushliteral(s_, "onShown");
+  lua_pushliteral(s_, "canPop");
 
   if (lua_gettable(s_, -2) == LUA_TFUNCTION) {
+    // If we got a callback instead of a value, then invoke it to turn it into
+    // value.
     lua_pushvalue(s_, -2);
-    lua::CallProtected(s_, 1, 0);
-  } else {
-    lua_pop(s_, 1);
+    lua::CallProtected(s_, 1, 1);
   }
+  bool ret = lua_toboolean(s_, -1);
 
-  lua_pop(s_, 1);
+  lua_pop(s_, 2);
+  return ret;
 }
 
-auto Lua::onHidden() -> void {
+auto Lua::SetObjRef(lua_State* s) -> void {
+  assert(s_ == nullptr);
+  s_ = s;
+  obj_ref_ = luaL_ref(s, LUA_REGISTRYINDEX);
+}
+
+auto Lua::callMethod(std::string name) -> void {
   if (!s_ || !obj_ref_) {
     return;
   }
   lua_rawgeti(s_, LUA_REGISTRYINDEX, *obj_ref_);
-  lua_pushliteral(s_, "onHidden");
+  lua_pushlstring(s_, name.data(), name.size());
 
   if (lua_gettable(s_, -2) == LUA_TFUNCTION) {
     lua_pushvalue(s_, -2);
@@ -61,29 +80,28 @@ auto Lua::onHidden() -> void {
   lua_pop(s_, 1);
 }
 
-auto Lua::canPop() -> bool {
+auto Lua::forEachBinding(std::function<void(lua::Binding*)> fn) -> void {
   if (!s_ || !obj_ref_) {
-    return true;
+    return;
   }
   lua_rawgeti(s_, LUA_REGISTRYINDEX, *obj_ref_);
-  lua_pushliteral(s_, "canPop");
+  lua_pushliteral(s_, "bindings");
 
-  if (lua_gettable(s_, -2) == LUA_TFUNCTION) {
-    // If we got a callback instead of a value, then invoke it to turn it into
-    // value.
-    lua_pushvalue(s_, -2);
-    lua::CallProtected(s_, 1, 1);
+  if (lua_gettable(s_, -2) != LUA_TTABLE) {
+    lua_pop(s_, 2);
+    return;
   }
-  bool ret = lua_toboolean(s_, -1);
 
-  lua_pop(s_, 2);
-  return ret;
-}
+  lua_pushnil(s_);
+  while (lua_next(s_, -2) != 0) {
+    lua::Binding* b = lua::Binding::get(s_, -1);
+    if (b) {
+      std::invoke(fn, b);
+    }
+    lua_pop(s_, 1);
+  }
 
-auto Lua::SetObjRef(lua_State* s) -> void {
-  assert(s_ == nullptr);
-  s_ = s;
-  obj_ref_ = luaL_ref(s, LUA_REGISTRYINDEX);
+  lua_pop(s_, 2);
 }
 
 }  // namespace screens

src/ui/ui_fsm.cpp

@@ -132,13 +132,13 @@ lua::Property UiState::sPlaybackPlaying{
 lua::Property UiState::sPlaybackTrack{};
 lua::Property UiState::sPlaybackPosition{
     0, [](const lua::LuaValue& val) {
-      int current_val = std::get<int>(sPlaybackPosition.Get());
+      int current_val = std::get<int>(sPlaybackPosition.get());
       if (!std::holds_alternative<int>(val)) {
         return false;
       }
       int new_val = std::get<int>(val);
       if (current_val != new_val) {
-        auto track = sPlaybackTrack.Get();
+        auto track = sPlaybackTrack.get();
         if (!std::holds_alternative<audio::TrackInfo>(track)) {
           return false;
         }
@@ -320,20 +320,20 @@ int UiState::PopScreen() {
 void UiState::react(const system_fsm::KeyLockChanged& ev) {
   sDisplay->SetDisplayOn(!ev.locking);
   sInput->lock(ev.locking);
-  sLockSwitch.Update(ev.locking);
+  sLockSwitch.setDirect(ev.locking);
 }
 
 void UiState::react(const system_fsm::SamdUsbStatusChanged& ev) {
-  sUsbMassStorageBusy.Update(ev.new_status ==
-                             drivers::Samd::UsbStatus::kAttachedBusy);
+  sUsbMassStorageBusy.setDirect(ev.new_status ==
+                                drivers::Samd::UsbStatus::kAttachedBusy);
 }
 
 void UiState::react(const database::event::UpdateStarted&) {
-  sDatabaseUpdating.Update(true);
+  sDatabaseUpdating.setDirect(true);
 }
 
 void UiState::react(const database::event::UpdateFinished&) {
-  sDatabaseUpdating.Update(false);
+  sDatabaseUpdating.setDirect(false);
 }
 
 void UiState::react(const internal::DismissAlerts&) {
@@ -341,46 +341,46 @@ void UiState::react(const internal::DismissAlerts&) {
 }
 
 void UiState::react(const system_fsm::BatteryStateChanged& ev) {
-  sBatteryPct.Update(static_cast<int>(ev.new_state.percent));
-  sBatteryMv.Update(static_cast<int>(ev.new_state.millivolts));
-  sBatteryCharging.Update(ev.new_state.is_charging);
+  sBatteryPct.setDirect(static_cast<int>(ev.new_state.percent));
+  sBatteryMv.setDirect(static_cast<int>(ev.new_state.millivolts));
+  sBatteryCharging.setDirect(ev.new_state.is_charging);
 }
 
 void UiState::react(const audio::QueueUpdate&) {
   auto& queue = sServices->track_queue();
-  sQueueSize.Update(static_cast<int>(queue.totalSize()));
+  sQueueSize.setDirect(static_cast<int>(queue.totalSize()));
 
   int current_pos = queue.currentPosition();
   if (queue.current()) {
     current_pos++;
   }
-  sQueuePosition.Update(current_pos);
-  sQueueRandom.Update(queue.random());
-  sQueueRepeat.Update(queue.repeat());
-  sQueueReplay.Update(queue.replay());
+  sQueuePosition.setDirect(current_pos);
+  sQueueRandom.setDirect(queue.random());
+  sQueueRepeat.setDirect(queue.repeat());
+  sQueueReplay.setDirect(queue.replay());
 }
 
 void UiState::react(const audio::PlaybackUpdate& ev) {
   if (ev.current_track) {
-    sPlaybackTrack.Update(*ev.current_track);
+    sPlaybackTrack.setDirect(*ev.current_track);
   } else {
-    sPlaybackTrack.Update(std::monostate{});
+    sPlaybackTrack.setDirect(std::monostate{});
   }
-  sPlaybackPlaying.Update(!ev.paused);
-  sPlaybackPosition.Update(static_cast<int>(ev.track_position.value_or(0)));
+  sPlaybackPlaying.setDirect(!ev.paused);
+  sPlaybackPosition.setDirect(static_cast<int>(ev.track_position.value_or(0)));
 }
 
 void UiState::react(const audio::VolumeChanged& ev) {
-  sVolumeCurrentPct.Update(static_cast<int>(ev.percent));
-  sVolumeCurrentDb.Update(static_cast<int>(ev.db));
+  sVolumeCurrentPct.setDirect(static_cast<int>(ev.percent));
+  sVolumeCurrentDb.setDirect(static_cast<int>(ev.db));
 }
 
 void UiState::react(const audio::VolumeBalanceChanged& ev) {
-  sVolumeLeftBias.Update(ev.left_bias);
+  sVolumeLeftBias.setDirect(ev.left_bias);
 }
 
 void UiState::react(const audio::VolumeLimitChanged& ev) {
-  sVolumeLimit.Update(ev.new_limit_db);
+  sVolumeLimit.setDirect(ev.new_limit_db);
 }
 
 void UiState::react(const system_fsm::BluetoothEvent& ev) {
@@ -388,19 +388,19 @@ void UiState::react(const system_fsm::BluetoothEvent& ev) {
   auto dev = bt.ConnectedDevice();
   switch (ev.event) {
     case drivers::bluetooth::Event::kKnownDevicesChanged:
-      sBluetoothDevices.Update(bt.KnownDevices());
+      sBluetoothDevices.setDirect(bt.KnownDevices());
       break;
     case drivers::bluetooth::Event::kConnectionStateChanged:
-      sBluetoothConnected.Update(bt.IsConnected());
+      sBluetoothConnected.setDirect(bt.IsConnected());
       if (dev) {
-        sBluetoothPairedDevice.Update(drivers::bluetooth::Device{
+        sBluetoothPairedDevice.setDirect(drivers::bluetooth::Device{
             .address = dev->mac,
             .name = {dev->name.data(), dev->name.size()},
             .class_of_device = 0,
             .signal_strength = 0,
         });
       } else {
-        sBluetoothPairedDevice.Update(std::monostate{});
+        sBluetoothPairedDevice.setDirect(std::monostate{});
       }
       break;
     case drivers::bluetooth::Event::kPreferredDeviceChanged:
@@ -428,7 +428,7 @@ void Splash::react(const system_fsm::BootComplete& ev) {
   themes::Theme::instance()->Apply();
 
   int brightness = sServices->nvs().ScreenBrightness();
-  sDisplayBrightness.Update(brightness);
+  sDisplayBrightness.setDirect(brightness);
   sDisplay->SetBrightness(brightness);
 
   sDeviceFactory = std::make_unique<input::DeviceFactory>(sServices);
@@ -530,12 +530,12 @@ void Lua::entry() {
                                    {"msc_busy", &sUsbMassStorageBusy},
                                });
 
-    sDatabaseAutoUpdate.Update(sServices->nvs().DbAutoIndex());
+    sDatabaseAutoUpdate.setDirect(sServices->nvs().DbAutoIndex());
 
     auto bt = sServices->bluetooth();
-    sBluetoothEnabled.Update(bt.IsEnabled());
-    sBluetoothConnected.Update(bt.IsConnected());
-    sBluetoothDevices.Update(bt.KnownDevices());
+    sBluetoothEnabled.setDirect(bt.IsEnabled());
+    sBluetoothConnected.setDirect(bt.IsConnected());
+    sBluetoothDevices.setDirect(bt.KnownDevices());
 
     sCurrentScreen.reset();
     sLua->RunScript("/sdcard/config.lua");