Use an mutable struct + const instead of an immutable class

src/database/database.cpp

@@ -202,8 +202,8 @@ auto Database::Update() -> std::future<void> {
           continue;
         }
 
-        if (track->is_tombstoned()) {
-          ESP_LOGW(kTag, "skipping tombstoned %lx", track->id());
+        if (track->is_tombstoned) {
+          ESP_LOGW(kTag, "skipping tombstoned %lx", track->id);
           it->Next();
           continue;
         }
@@ -212,25 +212,28 @@ auto Database::Update() -> std::future<void> {
         FILINFO info;
         {
           auto lock = drivers::acquire_spi();
-          res = f_stat(track->filepath().c_str(), &info);
+          res = f_stat(track->filepath.c_str(), &info);
         }
 
         std::pair<uint16_t, uint16_t> modified_at{0, 0};
         if (res == FR_OK) {
           modified_at = {info.fdate, info.ftime};
         }
-        if (modified_at == track->modified_at()) {
+        if (modified_at == track->modified_at) {
           newest_track = std::max(modified_at, newest_track);
+        } else {
+          track->modified_at = modified_at;
         }
 
         std::shared_ptr<TrackTags> tags =
-            tag_parser_.ReadAndParseTags(track->filepath());
+            tag_parser_.ReadAndParseTags(track->filepath);
         if (!tags || tags->encoding() == Container::kUnsupported) {
           // We couldn't read the tags for this track. Either they were
           // malformed, or perhaps the file is missing. Either way, tombstone
           // this record.
-          ESP_LOGW(kTag, "entombing missing #%lx", track->id());
-          dbPutTrackData(track->Entomb().UpdateModifiedAt(modified_at));
+          ESP_LOGW(kTag, "entombing missing #%lx", track->id);
+          track->is_tombstoned = true;
+          dbPutTrackData(*track);
           it->Next();
           continue;
         }
@@ -239,15 +242,15 @@ auto Database::Update() -> std::future<void> {
         // location. All that's left to do is update any metadata about it.
 
         uint64_t new_hash = tags->Hash();
-        if (new_hash != track->tags_hash()) {
+        if (new_hash != track->tags_hash) {
           // This track's tags have changed. Since the filepath is exactly the
           // same, we assume this is a legitimate correction. Update the
           // database.
-          ESP_LOGI(kTag, "updating hash (%llx -> %llx)", track->tags_hash(),
+          ESP_LOGI(kTag, "updating hash (%llx -> %llx)", track->tags_hash,
                    new_hash);
-          dbPutTrackData(
-              track->UpdateHash(new_hash).UpdateModifiedAt(modified_at));
-          dbPutHash(new_hash, track->id());
+          track->tags_hash = new_hash;
+          dbPutTrackData(*track);
+          dbPutHash(new_hash, track->id);
         }
 
         Track t{track, tags};
@@ -274,7 +277,6 @@ auto Database::Update() -> std::future<void> {
 
       std::pair<uint16_t, uint16_t> modified{info.fdate, info.ftime};
       if (modified < newest_track) {
-        ESP_LOGI(kTag, "skipping old file");
         return;
       }
 
@@ -299,7 +301,11 @@ auto Database::Update() -> std::future<void> {
         TrackId id = dbMintNewTrackId();
         ESP_LOGI(kTag, "recording new 0x%lx", id);
 
-        auto data = std::make_shared<TrackData>(id, path, hash, modified);
+        auto data = std::make_shared<TrackData>();
+        data->id = id;
+        data->filepath = path;
+        data->tags_hash = hash;
+        data->modified_at = modified;
 
         dbPutTrackData(*data);
         dbPutHash(hash, id);
@@ -311,22 +317,27 @@ auto Database::Update() -> std::future<void> {
       std::shared_ptr<TrackData> existing_data = dbGetTrackData(*existing_hash);
       if (!existing_data) {
         // We found a hash that matches, but there's no data record? Weird.
-        auto new_data =
-            std::make_shared<TrackData>(*existing_hash, path, hash, modified);
+        auto new_data = std::make_shared<TrackData>();
+        new_data->id = dbMintNewTrackId();
+        new_data->filepath = path;
+        new_data->tags_hash = hash;
+        new_data->modified_at = modified;
         dbPutTrackData(*new_data);
         auto t = std::make_shared<Track>(new_data, tags);
         dbCreateIndexesForTrack(*t);
         return;
       }
 
-      if (existing_data->is_tombstoned()) {
-        ESP_LOGI(kTag, "exhuming track %lu", existing_data->id());
-        dbPutTrackData(existing_data->Exhume(path));
+      if (existing_data->is_tombstoned) {
+        ESP_LOGI(kTag, "exhuming track %lu", existing_data->id);
+        existing_data->is_tombstoned = false;
+        existing_data->modified_at = modified;
+        dbPutTrackData(*existing_data);
         auto t = std::make_shared<Track>(existing_data, tags);
         dbCreateIndexesForTrack(*t);
-      } else if (existing_data->filepath() != path) {
+      } else if (existing_data->filepath != path) {
         ESP_LOGW(kTag, "tag hash collision for %s and %s",
-                 existing_data->filepath().c_str(), path.c_str());
+                 existing_data->filepath.c_str(), path.c_str());
         ESP_LOGI(kTag, "hash components: %s, %s, %s",
                  tags->at(Tag::kTitle).value_or("no title").c_str(),
                  tags->at(Tag::kArtist).value_or("no artist").c_str(),
@@ -343,7 +354,7 @@ auto Database::GetTrackPath(TrackId id)
       [=, this]() -> std::optional<std::pmr::string> {
         auto track_data = dbGetTrackData(id);
         if (track_data) {
-          return track_data->filepath();
+          return track_data->filepath;
         }
         return {};
       });
@@ -353,11 +364,11 @@ auto Database::GetTrack(TrackId id) -> std::future<std::shared_ptr<Track>> {
   return worker_task_->Dispatch<std::shared_ptr<Track>>(
       [=, this]() -> std::shared_ptr<Track> {
         std::shared_ptr<TrackData> data = dbGetTrackData(id);
-        if (!data || data->is_tombstoned()) {
+        if (!data || data->is_tombstoned) {
           return {};
         }
         std::shared_ptr<TrackTags> tags =
-            tag_parser_.ReadAndParseTags(data->filepath());
+            tag_parser_.ReadAndParseTags(data->filepath);
         if (!tags) {
           return {};
         }
@@ -505,10 +516,10 @@ auto Database::dbEntomb(TrackId id, uint64_t hash) -> void {
 }
 
 auto Database::dbPutTrackData(const TrackData& s) -> void {
-  std::string key = EncodeDataKey(s.id());
+  std::string key = EncodeDataKey(s.id);
   std::string val = EncodeDataValue(s);
   if (!db_->Put(leveldb::WriteOptions(), key, val).ok()) {
-    ESP_LOGE(kTag, "failed to write data for #%lx", s.id());
+    ESP_LOGE(kTag, "failed to write data for #%lx", s.id);
   }
 }
 
@@ -682,11 +693,11 @@ auto Database::ParseRecord<Track>(const leveldb::Slice& key,
                                   const leveldb::Slice& val)
     -> std::shared_ptr<Track> {
   std::shared_ptr<TrackData> data = ParseDataValue(val);
-  if (!data || data->is_tombstoned()) {
+  if (!data || data->is_tombstoned) {
     return {};
   }
   std::shared_ptr<TrackTags> tags =
-      tag_parser_.ReadAndParseTags(data->filepath());
+      tag_parser_.ReadAndParseTags(data->filepath);
   if (!tags) {
     return {};
   }

src/database/include/track.hpp

@@ -97,8 +97,8 @@ class TrackTags {
 };
 
 /*
- * Immutable owning container for all of the metadata we store for a particular
- * track. This includes two main kinds of metadata:
+ * Owning container for all of the metadata we store for a particular track.
+ * This includes two main kinds of metadata:
  *  1. static(ish) attributes, such as the id, path on disk, hash of the tags
  *  2. dynamic attributes, such as the number of times this track has been
  *  played.
@@ -113,66 +113,25 @@ class TrackTags {
  * properly restore dynamic attributes (such as play count) if the track later
  * re-appears on disk.
  */
-class TrackData {
- private:
-  const TrackId id_;
-  const std::pmr::string filepath_;
-  const uint64_t tags_hash_;
-  const bool is_tombstoned_;
-  const std::pair<uint16_t, uint16_t> modified_at_;
-
+struct TrackData {
  public:
-  /* Constructor used when adding new tracks to the database. */
-  TrackData(TrackId id,
-            const std::pmr::string& path,
-            uint64_t hash,
-            std::pair<uint16_t, uint16_t> modified_at)
-      : id_(id),
-        filepath_(path, &memory::kSpiRamResource),
-        tags_hash_(hash),
-        is_tombstoned_(false),
-        modified_at_(modified_at) {}
-
-  TrackData(TrackId id,
-            const std::pmr::string& path,
-            uint64_t hash,
-            bool is_tombstoned,
-            std::pair<uint16_t, uint16_t> modified_at)
-      : id_(id),
-        filepath_(path, &memory::kSpiRamResource),
-        tags_hash_(hash),
-        is_tombstoned_(is_tombstoned),
-        modified_at_(modified_at) {}
+  TrackData()
+      : id(0),
+        filepath(&memory::kSpiRamResource),
+        tags_hash(0),
+        is_tombstoned(false),
+        modified_at() {}
+
+  TrackId id;
+  std::pmr::string filepath;
+  uint64_t tags_hash;
+  bool is_tombstoned;
+  std::pair<uint16_t, uint16_t> modified_at;
 
   TrackData(TrackData&& other) = delete;
   TrackData& operator=(TrackData& other) = delete;
 
   bool operator==(const TrackData&) const = default;
-
-  auto id() const -> TrackId { return id_; }
-  auto filepath() const -> std::pmr::string { return filepath_; }
-  auto tags_hash() const -> uint64_t { return tags_hash_; }
-  auto is_tombstoned() const -> bool { return is_tombstoned_; }
-  auto modified_at() const -> std::pair<uint16_t, uint16_t> {
-    return modified_at_;
-  }
-
-  auto UpdateHash(uint64_t new_hash) const -> TrackData;
-
-  /*
-   * Marks this track data as a 'tombstone'. Tombstoned tracks are not playable,
-   * and should not generally be shown to users.
-   */
-  auto Entomb() const -> TrackData;
-
-  /*
-   * Clears the tombstone bit of this track, and updates the path to reflect its
-   * new location.
-   */
-  auto Exhume(const std::pmr::string& new_path) const -> TrackData;
-
-  auto UpdateModifiedAt(const std::pair<uint16_t, uint16_t>&) const
-      -> TrackData;
 };
 
 /*
@@ -199,7 +158,7 @@ class Track {
   auto TitleOrFilename() const -> std::pmr::string;
 
  private:
-  std::shared_ptr<TrackData> data_;
+  std::shared_ptr<const TrackData> data_;
   std::shared_ptr<TrackTags> tags_;
 };
 

src/database/index.cpp

@@ -89,7 +89,7 @@ auto Index(const IndexInfo& info, const Track& t, leveldb::WriteBatch* batch)
     // If this is the last component, then we should also fill in the track id
     // and title.
     if (i == info.components.size() - 1) {
-      key.track = t.data().id();
+      key.track = t.data().id;
       value = t.TitleOrFilename();
     }
 

src/database/records.cpp

@@ -63,12 +63,12 @@ auto EncodeDataKey(const TrackId& id) -> std::string {
 
 auto EncodeDataValue(const TrackData& track) -> std::string {
   cppbor::Array val{
-      cppbor::Uint{track.id()},
-      cppbor::Tstr{track.filepath()},
-      cppbor::Uint{track.tags_hash()},
-      cppbor::Bool{track.is_tombstoned()},
-      cppbor::Uint{track.modified_at().first},
-      cppbor::Uint{track.modified_at().second},
+      cppbor::Uint{track.id},
+      cppbor::Tstr{track.filepath},
+      cppbor::Uint{track.tags_hash},
+      cppbor::Bool{track.is_tombstoned},
+      cppbor::Uint{track.modified_at.first},
+      cppbor::Uint{track.modified_at.second},
   };
   return val.toString();
 }
@@ -88,16 +88,15 @@ auto ParseDataValue(const leveldb::Slice& slice) -> std::shared_ptr<TrackData> {
       vals->get(5)->type() != cppbor::UINT) {
     return {};
   }
-  TrackId id = vals->get(0)->asUint()->unsignedValue();
-  auto path = vals->get(1)->asViewTstr()->view();
-  uint64_t hash = vals->get(2)->asUint()->unsignedValue();
-  bool tombstoned = vals->get(3)->asBool()->value();
-  auto modified_at = std::make_pair<uint16_t, uint16_t>(
+  auto res = std::make_shared<TrackData>();
+  res->id = vals->get(0)->asUint()->unsignedValue();
+  res->filepath = vals->get(1)->asViewTstr()->view();
+  res->tags_hash = vals->get(2)->asUint()->unsignedValue();
+  res->is_tombstoned = vals->get(3)->asBool()->value();
+  res->modified_at = std::make_pair<uint16_t, uint16_t>(
       vals->get(4)->asUint()->unsignedValue(),
       vals->get(5)->asUint()->unsignedValue());
-  return std::make_shared<TrackData>(id,
-                                     std::pmr::string{path.data(), path.size()},
-                                     hash, tombstoned, modified_at);
+  return res;
 }
 
 /* 'H/ 0xBEEF' */

src/database/track.cpp

@@ -53,33 +53,16 @@ auto TrackTags::Hash() const -> uint64_t {
   return komihash_stream_final(&stream);
 }
 
-auto TrackData::UpdateHash(uint64_t new_hash) const -> TrackData {
-  return TrackData(id_, filepath_, new_hash, is_tombstoned_, modified_at_);
-}
-
-auto TrackData::Entomb() const -> TrackData {
-  return TrackData(id_, filepath_, tags_hash_, true, modified_at_);
-}
-
-auto TrackData::Exhume(const std::pmr::string& new_path) const -> TrackData {
-  return TrackData(id_, new_path, tags_hash_, false, modified_at_);
-}
-
-auto TrackData::UpdateModifiedAt(
-    const std::pair<uint16_t, uint16_t>& new_time) const -> TrackData {
-  return TrackData(id_, filepath_, tags_hash_, false, new_time);
-}
-
 auto Track::TitleOrFilename() const -> std::pmr::string {
   auto title = tags().at(Tag::kTitle);
   if (title) {
     return *title;
   }
-  auto start = data().filepath().find_last_of('/');
+  auto start = data().filepath.find_last_of('/');
   if (start == std::pmr::string::npos) {
-    return data().filepath();
+    return data().filepath;
   }
-  return data().filepath().substr(start);
+  return data().filepath.substr(start);
 }
 
 }  // namespace database

src/ui/screen_playing.cpp

@@ -173,7 +173,7 @@ Playing::Playing(models::TopBar& top_bar_model,
         if (!id) {
           return;
         }
-        if (current_track_.get() && current_track_.get()->data().id() == *id) {
+        if (current_track_.get() && current_track_.get()->data().id == *id) {
           return;
         }
         auto db = db_.lock();