Remove reference to file in AudioProperties and Tag

[kalouantonis]

Previously a handle to File was kept around in Tag and AudioProperties
so that the raw file pointer wouldn't be dropped while it was being used.

This made it difficult to just pass the tag around as a user of the API,
due to having to keep the file object alive as well.

For example, this would result in a lifetime error because the file object
does not live long enough:

taglib::File::new(path).and_then(|f| f.tag())

In this change the Tag and AudioProperties are created when request from
the file, E.g. file.tag() and there is no raw pointers being passed around.

If the user wishes to save new tags they pass the tag struct in to the save
function directly instead (AudioProperties can't be changed).

This fixes the issue for PR #7 causing a failure because of the file pointer deallocation. It also (unfortunately) includes the changes proposed in PR #8 as I was doing both of the changes concurrently.

[kalouantonis]

Looks like this PR might sit here for a while based on my observation of the others.

If anyone is interested in my changes feel free to use my repository in the meantime.

[cdown]

Looks good! After manual merge of #8 with changes, I need to tidy this up before merge which might take a bit.

[cdown]

Hmm, either I've messed up doing manual merge on your patch to master, or something is wrong with the patch itself, because after .save(), tags look corrupted. You can reproduce this with the test_set_tag test:

diff --git examples/tagreader.rs examples/tagreader.rs
index b49f809..57d76a3 100644
--- examples/tagreader.rs
+++ examples/tagreader.rs
@@ -20,13 +20,13 @@ pub fn main() {
         match file.tag() {
             Ok(t) => {
                 println!("-- TAG --");
-                println!("title   - {}", t.title().unwrap_or_default());
-                println!("artist  - {}", t.artist().unwrap_or_default());
-                println!("album   - {}", t.album().unwrap_or_default());
-                println!("year    - {}", t.year().unwrap_or_default());
-                println!("comment - {}", t.comment().unwrap_or_default());
-                println!("track   - {}", t.track().unwrap_or_default());
-                println!("genre   - {}", t.genre().unwrap_or_default());
+                println!("title   - {}", t.title.unwrap_or_default());
+                println!("artist  - {}", t.artist.unwrap_or_default());
+                println!("album   - {}", t.album.unwrap_or_default());
+                println!("year    - {}", t.year.unwrap_or_default());
+                println!("comment - {}", t.comment.unwrap_or_default());
+                println!("track   - {}", t.track.unwrap_or_default());
+                println!("genre   - {}", t.genre.unwrap_or_default());
             }
             Err(e) => {
                 println!("No available tags for {} (error: {:?})", arg, e);
@@ -35,13 +35,13 @@ pub fn main() {
 
         match file.audioproperties() {
             Ok(p) => {
-                let secs = p.length() % 60;
-                let mins = (p.length() - secs) / 60;
+                let secs = p.length % 60;
+                let mins = (p.length - secs) / 60;
 
                 println!("-- AUDIO --");
-                println!("bitrate     - {}", p.bitrate());
-                println!("sample rate - {}", p.samplerate());
-                println!("channels    - {}", p.channels());
+                println!("bitrate     - {}", p.bitrate);
+                println!("sample rate - {}", p.samplerate);
+                println!("channels    - {}", p.channels);
                 println!("length      - {}m:{}s", mins, secs);
             }
             Err(e) => {
diff --git src/lib.rs src/lib.rs
index d920d6d..b31c924 100644
--- src/lib.rs
+++ src/lib.rs
@@ -32,166 +32,85 @@ use sys as ll;
 
 fn c_str_to_str(c_str: *const c_char) -> Option<String> {
     if c_str.is_null() {
-        None
-    } else {
-        let bytes = unsafe { CStr::from_ptr(c_str).to_bytes() };
-
-        if bytes.is_empty() { None } else { Some(String::from_utf8_lossy(bytes).to_string()) }
+        return None;
     }
+
+    let bytes = unsafe { CStr::from_ptr(c_str).to_bytes() };
+    if bytes.is_empty() { None } else { Some(String::from_utf8_lossy(bytes).to_string()) }
 }
 
 fn u32_to_option(n: u32) -> Option<u32> {
     if n == 0 { None } else { Some(n) }
 }
 
+fn str_to_c_str(s: Option<String>) -> *const c_char {
+    let s = s.unwrap_or("".into());
+
+    CString::new(s.as_str()).unwrap().as_ptr()
+}
+
 /// A representation of an audio file, with meta-data and properties.
 pub struct File {
     raw: *mut ll::TagLib_File,
 }
 
-/// The abstract meta-data container for audio files
-///
-/// Each `Tag` instance can only be created by the `taglib::File::tag()`
-/// method.
-#[allow(dead_code)]
-pub struct Tag<'a> {
-    raw: *mut ll::TagLib_Tag,
-    file: &'a File,
-}
-
-/// Common audio file properties.
-///
-/// Instances of `AudioProperties` can only be created through the
-/// `taglib::File::audioproperties()` method.
-#[allow(dead_code)]
-pub struct AudioProperties<'a> {
-    raw: *const ll::TagLib_AudioProperties,
-    file: &'a File,
+/// Represents audio tag metadata.
+#[derive(Debug)]
+pub struct Tag {
+    /// The title of the track, if available.
+    pub title: Option<String>,
+    /// The album name, if available.
+    pub album: Option<String>,
+    /// The artist name, if available.
+    pub artist: Option<String>,
+    /// An additional comment, if available.
+    pub comment: Option<String>,
+    /// The genre, if any.
+    pub genre: Option<String>,
+    /// The year the track was created, if available.
+    pub year: Option<u32>,
+    /// The track number, if available.
+    pub track: Option<u32>,
 }
 
-impl<'a> Tag<'a> {
-    /// Returns the track name, if any.
-    pub fn title(&self) -> Option<String> {
-        let res = unsafe { ll::taglib_tag_title(self.raw) };
-        c_str_to_str(res)
-    }
-
-    /// Sets the track name.
-    pub fn set_title(&mut self, title: &str) {
-        let cs = CString::new(title).unwrap();
-        let s = cs.as_ptr();
-        unsafe {
-            ll::taglib_tag_set_title(self.raw, s);
-        }
-    }
-
-    /// Returns the artist name, if any.
-    pub fn artist(&self) -> Option<String> {
-        let res = unsafe { ll::taglib_tag_artist(self.raw) };
-        c_str_to_str(res)
-    }
-
-    /// Sets the artist name.
-    pub fn set_artist(&mut self, artist: &str) {
-        let cs = CString::new(artist).unwrap();
-        let s = cs.as_ptr();
-        unsafe {
-            ll::taglib_tag_set_artist(self.raw, s);
-        }
-    }
-
-    /// Returns the album name, if any.
-    pub fn album(&self) -> Option<String> {
-        let res = unsafe { ll::taglib_tag_album(self.raw) };
-        c_str_to_str(res)
-    }
-
-    /// Sets the album name.
-    pub fn set_album(&mut self, album: &str) {
-        let cs = CString::new(album).unwrap();
-        let s = cs.as_ptr();
-        unsafe {
-            ll::taglib_tag_set_album(self.raw, s);
-        }
-    }
-
-    /// Returns the track comment, if any.
-    pub fn comment(&self) -> Option<String> {
-        let res = unsafe { ll::taglib_tag_comment(self.raw) };
-        c_str_to_str(res)
-    }
-
-    /// Sets the track comment.
-    pub fn set_comment(&mut self, comment: &str) {
-        let cs = CString::new(comment).unwrap();
-        let s = cs.as_ptr();
-        unsafe {
-            ll::taglib_tag_set_comment(self.raw, s);
-        }
-    }
-
-    /// Returns the genre name, if any.
-    pub fn genre(&self) -> Option<String> {
-        let res = unsafe { ll::taglib_tag_genre(self.raw) };
-        c_str_to_str(res)
-    }
-
-    /// Sets the genre name.
-    pub fn set_genre(&mut self, genre: &str) {
-        let cs = CString::new(genre).unwrap();
-        let s = cs.as_ptr();
-        unsafe {
-            ll::taglib_tag_set_genre(self.raw, s);
-        }
-    }
-
-    /// Returns the year, if any.
-    pub fn year(&self) -> Option<u32> {
-        u32_to_option(unsafe { ll::taglib_tag_year(self.raw) as u32 })
-    }
-
-    /// Sets the year.
-    pub fn set_year(&mut self, year: u32) {
-        unsafe {
-            ll::taglib_tag_set_year(self.raw, year);
-        }
-    }
-
-    /// Returns the track number, if any.
-    pub fn track(&self) -> Option<u32> {
-        u32_to_option(unsafe { ll::taglib_tag_track(self.raw) as u32 })
-    }
-
-    /// Sets the track number.
-    pub fn set_track(&mut self, track: u32) {
-        unsafe {
-            ll::taglib_tag_set_track(self.raw, track);
+impl Tag {
+    unsafe fn new(raw: *const ll::TagLib_Tag) -> Tag {
+        Tag {
+            title: c_str_to_str(ll::taglib_tag_title(raw)),
+            album: c_str_to_str(ll::taglib_tag_album(raw)),
+            artist: c_str_to_str(ll::taglib_tag_artist(raw)),
+            comment: c_str_to_str(ll::taglib_tag_comment(raw)),
+            genre: c_str_to_str(ll::taglib_tag_genre(raw)),
+            year: u32_to_option(ll::taglib_tag_year(raw) as u32),
+            track: u32_to_option(ll::taglib_tag_track(raw) as u32),
         }
     }
 }
 
-impl<'a> AudioProperties<'a> {
-    /// Returns the length, in seconds, of the track.
-    pub fn length(&self) -> u32 {
-        unsafe { ll::taglib_audioproperties_length(self.raw) as u32 }
-    }
-
-    /// Returns the most appropriate bit rate for the track, in kB/s.
-    /// For constant bit rate formats, the returned value is the bit
-    /// rate of the file; for variable bit rate formats this is either
-    /// the average or the nominal bit rate.
-    pub fn bitrate(&self) -> u32 {
-        unsafe { ll::taglib_audioproperties_bitrate(self.raw) as u32 }
-    }
-
-    /// Returns the sample rate, in Hz.
-    pub fn samplerate(&self) -> u32 {
-        unsafe { ll::taglib_audioproperties_samplerate(self.raw) as u32 }
-    }
+/// Common audio file properties.
+#[derive(Debug)]
+pub struct AudioProperties {
+    /// The length, in seconds, of the track.
+    pub length: u32,
+    /// The most appropriate bit rate for the track, in KB/s.
+    /// For constant bit rate formats, the value is the bit rate of the file;
+    /// for variable bit rate formats this is either the average or the nominal
+    /// bit rate.
+    pub bitrate: u32,
+    /// The sample rate in Hz.
+    pub samplerate: u32,
+    /// The number of audio channels.
+    pub channels: u32,
+}
 
-    /// Returns the number of audio channels.
-    pub fn channels(&self) -> u32 {
-        unsafe { ll::taglib_audioproperties_channels(self.raw) as u32 }
+impl AudioProperties {
+    unsafe fn new(raw: *const ll::TagLib_AudioProperties) -> AudioProperties {
+        AudioProperties {
+            length: ll::taglib_audioproperties_length(raw) as u32,
+            bitrate: ll::taglib_audioproperties_bitrate(raw) as u32,
+            samplerate: ll::taglib_audioproperties_samplerate(raw) as u32,
+            channels: ll::taglib_audioproperties_channels(raw) as u32,
+        }
     }
 }
 
@@ -272,15 +191,13 @@ impl File {
 
     /// Returns the `taglib::Tag` instance for the given file.
     pub fn tag(&self) -> Result<Tag, FileError> {
-        let res = unsafe { ll::taglib_file_tag(self.raw) };
+        let raw = unsafe { ll::taglib_file_tag(self.raw) };
 
-        if res.is_null() {
+        if raw.is_null() {
             Err(FileError::NoAvailableTag)
         } else {
-            Ok(Tag {
-                raw: res,
-                file: self,
-            })
+            let tag = unsafe { Tag::new(raw) };
+            Ok(tag)
         }
     }
 
@@ -291,25 +208,34 @@ impl File {
 
     /// Returns the `taglib::AudioProperties` instance for the given file.
     pub fn audioproperties(&self) -> Result<AudioProperties, FileError> {
-        let res = unsafe { ll::taglib_file_audioproperties(self.raw) };
+        let raw = unsafe { ll::taglib_file_audioproperties(self.raw) };
 
-        if res.is_null() {
+        if raw.is_null() {
             Err(FileError::NoAvailableAudioProperties)
         } else {
-            Ok(AudioProperties {
-                raw: res,
-                file: self,
-            })
+            let props = unsafe { AudioProperties::new(raw) };
+            Ok(props)
         }
     }
 
-    /// Updates the meta-data of the file.
-    pub fn save(&self) -> bool {
-        unsafe { ll::taglib_file_save(self.raw) != 0 }
+    /// Write the given tag to the audio file.
+    pub fn save(&self, tag: Tag) -> bool {
+        unsafe {
+            let raw_tag = ll::taglib_file_tag(self.raw);
+            // if the user managed to get a tag to pass in then this should work
+            assert!(!raw_tag.is_null());
+            ll::taglib_tag_set_title(raw_tag, str_to_c_str(tag.title));
+            ll::taglib_tag_set_album(raw_tag, str_to_c_str(tag.album));
+            ll::taglib_tag_set_artist(raw_tag, str_to_c_str(tag.artist));
+            ll::taglib_tag_set_genre(raw_tag, str_to_c_str(tag.genre));
+            ll::taglib_tag_set_comment(raw_tag, str_to_c_str(tag.comment));
+            ll::taglib_tag_set_year(raw_tag, tag.year.unwrap_or(0));
+            ll::taglib_tag_set_track(raw_tag, tag.track.unwrap_or(0));
+            ll::taglib_file_save(self.raw) != 0
+        }
     }
 }
 
-
 /// Fixture creation:
 /// ffmpeg -t 0.01 -f s16le -i /dev/zero test.mp3
 /// kid3-cli -c 'set artist "Artist"' test.mp3
@@ -325,35 +251,35 @@ mod test {
     fn test_get_tag() {
         let file = File::new(TEST_MP3).unwrap();
         let tag = file.tag().unwrap();
-        assert_eq!(tag.artist().unwrap(), "Artist");
+        assert_eq!(tag.artist.unwrap(), "Artist");
     }
 
     #[test]
     fn test_get_pathbuf() {
         let file = File::new(PathBuf::from(TEST_MP3)).unwrap();
         let tag = file.tag().unwrap();
-        assert_eq!(tag.artist().unwrap(), "Artist");
+        assert_eq!(tag.artist.unwrap(), "Artist");
     }
 
     #[test]
     fn test_get_no_tag() {
         let file = File::new(TEST_MP3).unwrap();
         let tag = file.tag().unwrap();
-        assert_eq!(tag.album(), None);
+        assert_eq!(tag.album, None);
     }
 
     #[test]
     fn test_get_tag_new_type() {
         let file = File::new_type(TEST_MP3, FileType::MPEG).unwrap();
         let tag = file.tag().unwrap();
-        assert_eq!(tag.artist().unwrap(), "Artist");
+        assert_eq!(tag.artist.unwrap(), "Artist");
     }
 
     #[test]
     fn test_get_audioproperties() {
         let file = File::new(TEST_MP3).unwrap();
         let ap = file.audioproperties().unwrap();
-        assert_eq!(ap.length(), 0);
+        assert_eq!(ap.length, 0);
     }
 
     #[test]
@@ -363,10 +289,14 @@ mod test {
         let file = File::new(temp_fn).unwrap();
         let mut tag = file.tag().unwrap();
 
-        tag.set_artist("Not Artist");
-        file.save();
+        tag.artist = Some("Not Artist".to_string());
+        file.save(tag);
+
+        let file = File::new(temp_fn).unwrap();
+        let tag = file.tag().unwrap();
 
-        assert_eq!(tag.artist().unwrap(), "Not Artist");
+        println!("{:?}", tag);
+        assert_eq!(tag.artist.unwrap(), "Not Artist");
 
         fs::remove_file(temp_fn).unwrap();
     }

[cdown]

It also likely makes sense to have tag fields allow T, not just Option<T>. That's probably one thing the setters actually are useful for.