From 4f8c127da926bc1e1724e7686a42d37c1da0f563 Mon Sep 17 00:00:00 2001 From: jacqueline Date: Tue, 24 Oct 2023 12:50:03 +1100 Subject: [PATCH] Use an mutable struct + const instead of an immutable class --- src/database/database.cpp | 67 ++++++++++++++++++------------- src/database/include/track.hpp | 73 ++++++++-------------------------- src/database/index.cpp | 2 +- src/database/records.cpp | 27 ++++++------- src/database/track.cpp | 23 ++--------- src/ui/screen_playing.cpp | 2 +- 6 files changed, 73 insertions(+), 121 deletions(-) diff --git a/src/database/database.cpp b/src/database/database.cpp index 70bbb78a..0a092774 100644 --- a/src/database/database.cpp +++ b/src/database/database.cpp @@ -202,8 +202,8 @@ auto Database::Update() -> std::future { 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 { 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 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 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 { // 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 { std::pair 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 { TrackId id = dbMintNewTrackId(); ESP_LOGI(kTag, "recording new 0x%lx", id); - auto data = std::make_shared(id, path, hash, modified); + auto data = std::make_shared(); + 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 { std::shared_ptr 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(*existing_hash, path, hash, modified); + auto new_data = std::make_shared(); + 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(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(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 { 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> { return worker_task_->Dispatch>( [=, this]() -> std::shared_ptr { std::shared_ptr data = dbGetTrackData(id); - if (!data || data->is_tombstoned()) { + if (!data || data->is_tombstoned) { return {}; } std::shared_ptr 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(const leveldb::Slice& key, const leveldb::Slice& val) -> std::shared_ptr { std::shared_ptr data = ParseDataValue(val); - if (!data || data->is_tombstoned()) { + if (!data || data->is_tombstoned) { return {}; } std::shared_ptr tags = - tag_parser_.ReadAndParseTags(data->filepath()); + tag_parser_.ReadAndParseTags(data->filepath); if (!tags) { return {}; } diff --git a/src/database/include/track.hpp b/src/database/include/track.hpp index af5f53df..b07da9ba 100644 --- a/src/database/include/track.hpp +++ b/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 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 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 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 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 { - 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&) const - -> TrackData; }; /* @@ -199,7 +158,7 @@ class Track { auto TitleOrFilename() const -> std::pmr::string; private: - std::shared_ptr data_; + std::shared_ptr data_; std::shared_ptr tags_; }; diff --git a/src/database/index.cpp b/src/database/index.cpp index c7fd753a..4d1f7b06 100644 --- a/src/database/index.cpp +++ b/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(); } diff --git a/src/database/records.cpp b/src/database/records.cpp index 9d4dbd78..0619cd93 100644 --- a/src/database/records.cpp +++ b/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 { 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( + auto res = std::make_shared(); + 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( vals->get(4)->asUint()->unsignedValue(), vals->get(5)->asUint()->unsignedValue()); - return std::make_shared(id, - std::pmr::string{path.data(), path.size()}, - hash, tombstoned, modified_at); + return res; } /* 'H/ 0xBEEF' */ diff --git a/src/database/track.cpp b/src/database/track.cpp index 6fc891a4..d30264cd 100644 --- a/src/database/track.cpp +++ b/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& 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 diff --git a/src/ui/screen_playing.cpp b/src/ui/screen_playing.cpp index d7fda0b6..7d95cb7d 100644 --- a/src/ui/screen_playing.cpp +++ b/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();