Fix the "delete" function in scribbu text.

This patch will fix a bug in the `-d` flag to `scribbu text`, which is supposed to delete text frames. Previously, although the code would erase the given frame from all lookup tables, it would neglect to actually delete the frame from the vector of frames in each tag type. This commit fixes that.
sp1ff · Jan 10, 2024 · ca37564 · ca37564
1 parent 5d9e03a
commit ca37564
Show file tree

Hide file tree

Showing 7 changed files with 35 additions and 3 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -38,8 +38,8 @@ jobs:
           boost-dl: https://boostorg.jfrog.io/artifactory/main/release/1.74.0/source/boost_1_74_0.tar.bz2
         - os: macos-12
           boost: latest
-          boost-ver: 1_83_0
-          boost-dl: https://boostorg.jfrog.io/artifactory/main/release/1.83.0/source/boost_1_83_0.tar.bz2
+          boost-ver: 1_84_0
+          boost-dl: https://boostorg.jfrog.io/artifactory/main/release/1.84.0/source/boost_1_84_0.tar.bz2
         - os: ubuntu-22.04
           boost: oldest
           boost-ver: 1_63_0

diff --git a/NEWS b/NEWS
@@ -4,6 +4,8 @@ scribbu News -- history of user-visible changes -*- outline -*-
 
 ** Bug Fixes
 
+
+*** `scribbu text -d` doesn't actually delete the given frames
 *** Scheme function & types defined in C are now in module (scribbu)
 *** fixed several bugs in `write_id3v1_tag`
 

diff --git a/scribbu/id3v22.cc b/scribbu/id3v22.cc
@@ -380,6 +380,7 @@ scribbu::id3v2_2_tag::delete_frame(id3v2_text_frames id)
   if (p != frame_map_.end()) {
     std::ptrdiff_t idx = p->second;
     remove_frame_from_lookups(id3, idx);
+    frames_.erase(frames_.begin() + idx);
   }
 }
 

diff --git a/scribbu/id3v23.cc b/scribbu/id3v23.cc
@@ -759,6 +759,7 @@ scribbu::id3v2_3_tag::delete_frame(id3v2_text_frames id)
   if (p != frame_map_.end()) {
     std::ptrdiff_t idx = p->second;
     remove_frame_from_lookups(id3, idx);
+    frames_.erase(frames_.begin() + idx);
   }
 }
 

diff --git a/scribbu/id3v24.cc b/scribbu/id3v24.cc
@@ -833,6 +833,7 @@ scribbu::id3v2_4_tag::delete_frame(id3v2_text_frames id)
   if (p != frame_map_.end()) {
     std::ptrdiff_t idx = p->second;
     remove_frame_from_lookups(id3, idx);
+    frames_.erase(frames_.begin() + idx);
   }
 }
 

diff --git a/test/id3v22.cc b/test/id3v22.cc
@@ -377,11 +377,15 @@ BOOST_AUTO_TEST_CASE( test_id3v22_text_frames )
 
   id3v2_2_tag tag(ifs);
 
+  ostringstream stm;
+  BOOST_TEST_MESSAGE("Test ID3v2.2 tag has " << tag.num_frames() << " frames");
+
   BOOST_CHECK("Murley Braid Quartet" == tag.text(id3v2_text_frames::tpe1));
   tag.text(id3v2_text_frames::tpe1, "foo");
   BOOST_CHECK("foo" == tag.text(id3v2_text_frames::tpe1));
-  tag.delete_frame(id3v2_text_frames::tpe1);
   BOOST_CHECK( 9 == tag.num_frames() );
+  tag.delete_frame(id3v2_text_frames::tpe1);
+  BOOST_CHECK( 8 == tag.num_frames() );
 
   BOOST_CHECK( !tag.has_artist() );
 

diff --git a/test/test-text b/test/test-text
@@ -108,6 +108,27 @@ function test_create
     clean_testdir $here $text_testdir test_create
 }
 
+# Test that a frame is deleted when the `-d` argument is supplied
+function test_delete
+{
+    local out
+    local err
+    local stat
+
+    here=`pwd`
+    set_testdir ${srcdir} ${text_testdir} test_delete cerulean.mp3
+
+    run_text out err stat -d TPE3 cerulean.mp3
+    [ $stat -eq 0 ] || fail "test_delete: non-zero status on set artist"
+    [ -z "$err" ] || fail "test_delete: non-nil stderr on set artist ($err)"
+
+    conductor=$(${text_scribbu_dir}/scribbu dump cerulean.mp3|grep -F TPE3)
+    stat=$?
+    [ $stat -ne 0 ] || fail "test_delete: TPE3 appears to still be present"
+
+    clean_testdir $here $text_testdir test_delete
+}
+
 ###########################################################################
 # main
 ###########################################################################
@@ -185,3 +206,5 @@ text_scribbu_dir=`cd "$text_scribbu_dir"; pwd`
 text_testdir=`cd "$text_testdir"; pwd`
 smoke_tests
 test_create
+test_delete
+