Repository navigation
Conversation
track_impl::update() in the v2 and v3 engines built a fresh row from the snapshot and wrote it over the existing one, so every column the snapshot has no field for was reset to the default used for a new track. Reading a snapshot, changing the title and writing it back would detach the track's album art, reset its date added and clear its played flag. update() now reads the existing row and overwrites only the columns that come from the snapshot, keeping the rest as they were. Tests for v2 and v3 give a track album art, a date added and a played flag through the low-level track table, update it with a changed title, and check that the title changes and the other columns do not.
mr-smidge
left a comment
mr-smidge
left a comment
There was a problem hiding this comment.
Apologies for the slow turnaround time on this review... left a few small comments. Thank you!
| BOOST_DATA_TEST_CASE( | ||
| update__v2_unsnapshotted_columns__keeps, e::supported_v2_schemas, schema) |
There was a problem hiding this comment.
These new tests, because they interact with the low-level APIs, should go in v2/track_table_test.cpp and v3/track_table_test.cpp rather than here. This unit test class is meant to be exercising the high-level API "uniformly" across all supported schemas.
Please could you move them? Thanks.
| example_track_data_usage::create, schema); | ||
|
|
||
| BOOST_TEST_CHECKPOINT("(" << schema << ") Creating track..."); | ||
| auto track = db.create_track(snapshot); |
There was a problem hiding this comment.
Perhaps use the low-level API to create the track?
That way, you won't have to mix high-level and low-level calls to make the track entry.
Possibly it may make it easier to compare the DB row from before and after too.
| library.track().set_is_played(track.id(), true); | ||
|
|
||
| auto modified = track.snapshot(); | ||
| modified.title = "Updated Title"; |
There was a problem hiding this comment.
I wonder if you even need to change anything in the snapshot to prove that the round-tripping works properly for all the other fields...?
| BOOST_CHECK(row->title == std::optional<std::string>{"Updated Title"}); | ||
| BOOST_CHECK_EQUAL(row->album_art_id, album_art_id); | ||
| BOOST_CHECK(row->date_added == date_added); | ||
| BOOST_CHECK(row->is_played); |
There was a problem hiding this comment.
Right now you check that specific fields have not changed, with an assertion for each. But if more fields are added in future then this test could go stale.
If you use the low-level API to create the initial track row (see other review comment), then an alternative approach might be to deliberately erase the one or two fields you do expect to change (last_edit_time, any others?), and then use track_row::operator==() to ensure that nothing else has changed. What do you think?
| if (!performance_data_row_maybe) | ||
| throw djinterop::track_database_inconsistency{ | ||
| "No performance data row is present for track", id()}; |
There was a problem hiding this comment.
Is it possible to get a track imported into EngineDJ but not analyse it? Would that result in an entry in Track but not PerformanceData?
(I don't know the answer to this... I recall I did try once a few years ago but tracks always seemed to have a performance data entry...)
I don't think anything in this comment should block the PR, but I think this is what isAnalyzed is meant to represent.
There was a problem hiding this comment.
Yes, and Engine will then analyze the track on the mixer device and add PerformanceData to the database on the stick. At least on Prime 4 and Prime Go devices.
No worries! I'm ok holiday right now, will take care of this once I get back! Thanks for the review! |
The tests use the low-level track table, so they belong in the v2 and v3 track table tests rather than the schema-uniform high-level track test. Instead of checking the three columns that were lost, each test now updates the track with its own unchanged snapshot and compares the whole row before and after, so a column added later is covered too.
|
Thanks again for the review! I've pushed the changes:
One deviation from your suggestion: the track is created through the high-level API, not the low-level one. With a row from
Nothing is lost there, but the rows aren't equal. Creating the track through While looking into this I noticed two inconsistencies in the v2 example rows:
Happy to fix those in a separate PR if you like. |
track::update(const track_snapshot&)on Engine v2 and v3 libraries builds a fresh row from the snapshot and writes it over the existing one. Columns the snapshot has no field for are reset to the defaults used for a new track:albumArtIdgoes back to the no-artwork id,dateAddedbecomes the current time,isPlayedis cleared, and so on. So reading a snapshot, changing the title and writing it back detaches the track's cover and loses its history.With this change,
update()reads the existing row (and, on v3, thePerformanceDatarow) and overwrites only the columns that come from the snapshot. Everything else is kept as it was.Two consequences that may be worth a look in review.
isAnalyzedis now kept as well:update()used to set it on every call, but a snapshot has no field for it. Andupdate()now throwstrack_deletedwhen the track's row is gone (and, on v3,track_database_inconsistencywhen itsPerformanceDatarow is missing, assnapshot()does), where it used to update nothing without saying so.The new tests in
track_test.cpp, one for v2 schemas and one for v3, set album art, a date added and the played flag through the low-level track table, update the track with a changed title, and check that the title changes and the other three columns do not. Without the fix they fail on all three columns for every v2 and v3 schema.