From 3566b00596110d85060ee33e96286ec9b3924131 Mon Sep 17 00:00:00 2001 From: Urs Fleisch Date: Fri, 22 Aug 2025 13:27:07 +0200 Subject: [PATCH] Clean up attachments Avoid use of raw pointers. --- examples/matroskareader.cpp | 12 ++-- examples/matroskawriter.cpp | 10 ++-- taglib/matroska/ebml/ebmlmkattachments.cpp | 12 ++-- taglib/matroska/matroskaattachedfile.cpp | 50 +++++++++++++++- taglib/matroska/matroskaattachedfile.h | 70 ++++++++++++++++++++++ taglib/matroska/matroskaattachments.cpp | 54 +++++++++++++---- taglib/matroska/matroskaattachments.h | 20 +++++-- taglib/matroska/matroskafile.cpp | 39 ++++++------ 8 files changed, 210 insertions(+), 57 deletions(-) diff --git a/examples/matroskareader.cpp b/examples/matroskareader.cpp index 070fc75d..93a7a3c4 100644 --- a/examples/matroskareader.cpp +++ b/examples/matroskareader.cpp @@ -56,17 +56,17 @@ int main(int argc, char *argv[]) if(attachments) { const TagLib::Matroska::Attachments::AttachedFileList &list = attachments->attachedFileList(); printf("Found %u attachment(s)\n", list.size()); - for(auto attachedFile : list) { - PRINT_PRETTY("Filename", attachedFile->fileName().toCString(true)); - const TagLib::String &description = attachedFile->description(); + for(const auto &attachedFile : list) { + PRINT_PRETTY("Filename", attachedFile.fileName().toCString(true)); + const TagLib::String &description = attachedFile.description(); PRINT_PRETTY("Description", !description.isEmpty() ? description.toCString(true) : "None"); - const TagLib::String &mediaType = attachedFile->mediaType(); + const TagLib::String &mediaType = attachedFile.mediaType(); PRINT_PRETTY("Media Type", !mediaType.isEmpty() ? mediaType.toCString(false) : "None"); PRINT_PRETTY("Data Size", - TagLib::Utils::formatString("%u byte(s)",attachedFile->data().size()).toCString(false) + TagLib::Utils::formatString("%u byte(s)",attachedFile.data().size()).toCString(false) ); PRINT_PRETTY("UID", - TagLib::Utils::formatString("%llu",attachedFile->uid()).toCString(false) + TagLib::Utils::formatString("%llu",attachedFile.uid()).toCString(false) ); } } diff --git a/examples/matroskawriter.cpp b/examples/matroskawriter.cpp index 6ae11953..a5f762fc 100644 --- a/examples/matroskawriter.cpp +++ b/examples/matroskawriter.cpp @@ -41,11 +41,11 @@ int main(int argc, char *argv[]) TagLib::FileStream image(argv[2]); auto data = image.readBlock(image.length()); auto attachments = file.attachments(true); - auto attachedFile = new TagLib::Matroska::AttachedFile(); - attachedFile->setFileName("cover.jpg"); - attachedFile->setMediaType("image/jpeg"); - attachedFile->setData(data); - //attachedFile->setUID(5081000385627515072ull); + TagLib::Matroska::AttachedFile attachedFile; + attachedFile.setFileName("cover.jpg"); + attachedFile.setMediaType("image/jpeg"); + attachedFile.setData(data); + //attachedFile.setUID(5081000385627515072ull); attachments->addAttachedFile(attachedFile); file.save(); diff --git a/taglib/matroska/ebml/ebmlmkattachments.cpp b/taglib/matroska/ebml/ebmlmkattachments.cpp index ed8ab2b9..bc58b952 100644 --- a/taglib/matroska/ebml/ebmlmkattachments.cpp +++ b/taglib/matroska/ebml/ebmlmkattachments.cpp @@ -59,15 +59,15 @@ std::unique_ptr EBML::MkAttachments::parse() if(!(filename && data)) continue; - auto file = new Matroska::AttachedFile(); - file->setFileName(*filename); - file->setData(*data); + Matroska::AttachedFile file; + file.setFileName(*filename); + file.setData(*data); if(description) - file->setDescription(*description); + file.setDescription(*description); if(mediaType) - file->setMediaType(*mediaType); + file.setMediaType(*mediaType); if(uid) - file->setUID(uid); + file.setUID(uid); attachments->addAttachedFile(file); } diff --git a/taglib/matroska/matroskaattachedfile.cpp b/taglib/matroska/matroskaattachedfile.cpp index 01c99d31..06c6f4f3 100644 --- a/taglib/matroska/matroskaattachedfile.cpp +++ b/taglib/matroska/matroskaattachedfile.cpp @@ -1,4 +1,23 @@ -#include +/*************************************************************************** +* This library is free software; you can redistribute it and/or modify * + * it under the terms of the GNU Lesser General Public License version * + * 2.1 as published by the Free Software Foundation. * + * * + * This library is distributed in the hope that it will be useful, but * + * WITHOUT ANY WARRANTY; without even the implied warranty of * + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * + * Lesser General Public License for more details. * + * * + * You should have received a copy of the GNU Lesser General Public * + * License along with this library; if not, write to the Free Software * + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA * + * 02110-1301 USA * + * * + * Alternatively, this file is available under the Mozilla Public * + * License Version 1.1. You may obtain a copy of the License at * + * http://www.mozilla.org/MPL/ * + ***************************************************************************/ + #include "matroskaattachedfile.h" #include "tstring.h" #include "tbytevector.h" @@ -10,8 +29,6 @@ class Matroska::AttachedFile::AttachedFilePrivate public: AttachedFilePrivate() = default; ~AttachedFilePrivate() = default; - AttachedFilePrivate(const AttachedFilePrivate &) = delete; - AttachedFilePrivate &operator=(const AttachedFilePrivate &) = delete; String fileName; String description; String mediaType; @@ -19,12 +36,39 @@ public: UID uid = 0; }; +//////////////////////////////////////////////////////////////////////////////// +// public members +//////////////////////////////////////////////////////////////////////////////// + Matroska::AttachedFile::AttachedFile() : d(std::make_unique()) { } + +Matroska::AttachedFile::AttachedFile(const AttachedFile &other) : + d(std::make_unique(*other.d)) +{ +} + +Matroska::AttachedFile::AttachedFile(AttachedFile&& other) noexcept = default; + Matroska::AttachedFile::~AttachedFile() = default; +Matroska::AttachedFile &Matroska::AttachedFile::operator=(AttachedFile &&other) = default; + +Matroska::AttachedFile &Matroska::AttachedFile::operator=(const AttachedFile &other) +{ + AttachedFile(other).swap(*this); + return *this; +} + +void Matroska::AttachedFile::swap(AttachedFile &other) noexcept +{ + using std::swap; + + swap(d, other.d); +} + void Matroska::AttachedFile::setFileName(const String &fileName) { d->fileName = fileName; diff --git a/taglib/matroska/matroskaattachedfile.h b/taglib/matroska/matroskaattachedfile.h index c28e889e..14cb5475 100644 --- a/taglib/matroska/matroskaattachedfile.h +++ b/taglib/matroska/matroskaattachedfile.h @@ -21,28 +21,98 @@ #ifndef TAGLIB_MATROSKAATTACHEDFILE_H #define TAGLIB_MATROSKAATTACHEDFILE_H +#include #include "taglib_export.h" namespace TagLib { class String; class ByteVector; namespace Matroska { + //! Attached file embedded into a Matroska file. class TAGLIB_EXPORT AttachedFile { public: using UID = unsigned long long; AttachedFile(); + + /*! + * Construct an attached file as a copy of \a other. + */ + AttachedFile(const AttachedFile &other); + + /*! + * Construct an attached file moving from \a other. + */ + AttachedFile(AttachedFile &&other) noexcept; + + /*! + * Destroys this attached file. + */ ~AttachedFile(); + /*! + * Copies the contents of \a other into this object. + */ + AttachedFile &operator=(const AttachedFile &other); + + /*! + * Moves the contents of \a other into this object. + */ + AttachedFile &operator=(AttachedFile &&other); + + /*! + * Exchanges the content of the object with the content of \a other. + */ + void swap(AttachedFile &other) noexcept; + + /*! + * Set the \a fileName of the attached file. + */ void setFileName(const String &fileName); + + /*! + * Returns the filename of the attached file. + */ const String &fileName() const; + + /*! + * Set a human-friendly \a description for the attached file. + */ void setDescription(const String &description); + + /*! + * Returns the human-friendly description for the attached file. + */ const String &description() const; + + /*! + * Set the \a mediaType of the attached file. + */ void setMediaType(const String &mediaType); + + /*! + * Returns the media type of the attached file. + */ const String &mediaType() const; + + /*! + * Set the data of the attached file. + */ void setData(const ByteVector &data); + + /*! + * Returns the data of the attached file. + */ const ByteVector &data() const; + + /*! + * Set the \a uid representing the file, as random as possible. + */ void setUID(UID uid); + + /*! + * Returns the UID of the attached file. + */ UID uid() const; private: diff --git a/taglib/matroska/matroskaattachments.cpp b/taglib/matroska/matroskaattachments.cpp index b04ace77..b11eafb2 100644 --- a/taglib/matroska/matroskaattachments.cpp +++ b/taglib/matroska/matroskaattachments.cpp @@ -1,3 +1,23 @@ +/*************************************************************************** +* This library is free software; you can redistribute it and/or modify * + * it under the terms of the GNU Lesser General Public License version * + * 2.1 as published by the Free Software Foundation. * + * * + * This library is distributed in the hope that it will be useful, but * + * WITHOUT ANY WARRANTY; without even the implied warranty of * + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * + * Lesser General Public License for more details. * + * * + * You should have received a copy of the GNU Lesser General Public * + * License along with this library; if not, write to the Free Software * + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA * + * 02110-1301 USA * + * * + * Alternatively, this file is available under the Mozilla Public * + * License Version 1.1. You may obtain a copy of the License at * + * http://www.mozilla.org/MPL/ * + ***************************************************************************/ + #include "matroskaattachments.h" #include #include "matroskaattachedfile.h" @@ -18,27 +38,33 @@ public: ~AttachmentsPrivate() = default; AttachmentsPrivate(const AttachmentsPrivate &) = delete; AttachmentsPrivate &operator=(const AttachmentsPrivate &) = delete; - List files; + AttachedFileList files; }; +//////////////////////////////////////////////////////////////////////////////// +// public members +//////////////////////////////////////////////////////////////////////////////// + Matroska::Attachments::Attachments() : Element(static_cast(EBML::Element::Id::MkAttachments)), d(std::make_unique()) { - d->files.setAutoDelete(true); } + Matroska::Attachments::~Attachments() = default; -void Matroska::Attachments::addAttachedFile(AttachedFile *file) +void Matroska::Attachments::addAttachedFile(const AttachedFile& file) { d->files.append(file); } -void Matroska::Attachments::removeAttachedFile(AttachedFile *file) +void Matroska::Attachments::removeAttachedFile(unsigned long long uid) { - auto it = d->files.find(file); + auto it = std::find_if(d->files.begin(), d->files.end(), + [uid](const AttachedFile& file) { + return file.uid() == uid; + }); if(it != d->files.end()) { - delete *it; d->files.erase(it); } } @@ -53,6 +79,10 @@ const Matroska::Attachments::AttachedFileList &Matroska::Attachments::attachedFi return d->files; } +//////////////////////////////////////////////////////////////////////////////// +// private members +//////////////////////////////////////////////////////////////////////////////// + Matroska::Attachments::AttachedFileList &Matroska::Attachments::attachedFiles() { return d->files; @@ -61,21 +91,21 @@ Matroska::Attachments::AttachedFileList &Matroska::Attachments::attachedFiles() bool Matroska::Attachments::render() { EBML::MkAttachments attachments; - for(const auto attachedFile : d->files) { + for(const auto &attachedFile : std::as_const(d->files)) { auto attachedFileElement = EBML::make_unique_element(); // Filename auto fileNameElement = EBML::make_unique_element(); - fileNameElement->setValue(attachedFile->fileName()); + fileNameElement->setValue(attachedFile.fileName()); attachedFileElement->appendElement(std::move(fileNameElement)); // Media/MIME type auto mediaTypeElement = EBML::make_unique_element(); - mediaTypeElement->setValue(attachedFile->mediaType()); + mediaTypeElement->setValue(attachedFile.mediaType()); attachedFileElement->appendElement(std::move(mediaTypeElement)); // Description - const String &description = attachedFile->description(); + const String &description = attachedFile.description(); if(!description.isEmpty()) { auto descriptionElement = EBML::make_unique_element(); descriptionElement->setValue(description); @@ -84,12 +114,12 @@ bool Matroska::Attachments::render() // Data auto dataElement = EBML::make_unique_element(); - dataElement->setValue(attachedFile->data()); + dataElement->setValue(attachedFile.data()); attachedFileElement->appendElement(std::move(dataElement)); // UID auto uidElement = EBML::make_unique_element(); - AttachedFile::UID uid = attachedFile->uid(); + AttachedFile::UID uid = attachedFile.uid(); if(!uid) uid = EBML::randomUID(); uidElement->setValue(uid); diff --git a/taglib/matroska/matroskaattachments.h b/taglib/matroska/matroskaattachments.h index 4489cabb..b5ae7d1e 100644 --- a/taglib/matroska/matroskaattachments.h +++ b/taglib/matroska/matroskaattachments.h @@ -34,19 +34,31 @@ namespace TagLib { namespace Matroska { class AttachedFile; class File; + + //! Collection of attached files. class TAGLIB_EXPORT Attachments #ifndef DO_NOT_DOCUMENT : private Element #endif { public: - using AttachedFileList = List; + using AttachedFileList = List; + //! Construct attachments. Attachments(); - virtual ~Attachments(); - void addAttachedFile(AttachedFile *file); - void removeAttachedFile(AttachedFile *file); + //! Destroy attachements. + ~Attachments(); + + //! Add an attached file. + void addAttachedFile(const AttachedFile &file); + + //! Remove an attached file. + void removeAttachedFile(unsigned long long uid); + + //! Remove an attached file. void clear(); + + //! Get list of all attached files. const AttachedFileList &attachedFileList() const; private: diff --git a/taglib/matroska/matroskafile.cpp b/taglib/matroska/matroskafile.cpp index ac9c7fca..84ccd507 100644 --- a/taglib/matroska/matroskafile.cpp +++ b/taglib/matroska/matroskafile.cpp @@ -133,21 +133,18 @@ PropertyMap Matroska::File::setProperties(const PropertyMap &properties) namespace { - String keyForAttachedFile(const Matroska::AttachedFile *attachedFile) + String keyForAttachedFile(const Matroska::AttachedFile &attachedFile) { - if(!attachedFile) { - return {}; - } - if(attachedFile->mediaType().startsWith("image/")) { + if(attachedFile.mediaType().startsWith("image/")) { return "PICTURE"; } - if(!attachedFile->mediaType().isEmpty()) { - return attachedFile->mediaType(); + if(!attachedFile.mediaType().isEmpty()) { + return attachedFile.mediaType(); } - if(!attachedFile->fileName().isEmpty()) { - return attachedFile->fileName(); + if(!attachedFile.fileName().isEmpty()) { + return attachedFile.fileName(); } - return String::fromLongLong(attachedFile->uid()); + return String::fromLongLong(attachedFile.uid()); } unsigned long long stringToULongLong(const String &str, bool *ok) @@ -187,11 +184,11 @@ List Matroska::File::complexProperties(const String &key) const for(const auto &attachedFile : attachedFiles) { if(keyForAttachedFile(attachedFile) == key) { VariantMap property; - property.insert("data", attachedFile->data()); - property.insert("mimeType", attachedFile->mediaType()); - property.insert("description", attachedFile->description()); - property.insert("fileName", attachedFile->fileName()); - property.insert("uid", attachedFile->uid()); + property.insert("data", attachedFile.data()); + property.insert("mimeType", attachedFile.mediaType()); + property.insert("description", attachedFile.description()); + property.insert("fileName", attachedFile.fileName()); + property.insert("uid", attachedFile.uid()); props.append(property); } } @@ -226,12 +223,12 @@ bool Matroska::File::setComplexProperties(const String &key, const ListsetData(data); - attachedFile->setMediaType(mimeType); - attachedFile->setDescription(property.value("description").value()); - attachedFile->setFileName(fileName); - attachedFile->setUID(uid); + AttachedFile attachedFile; + attachedFile.setData(data); + attachedFile.setMediaType(mimeType); + attachedFile.setDescription(property.value("description").value()); + attachedFile.setFileName(fileName); + attachedFile.setUID(uid); d->attachments->addAttachedFile(attachedFile); } return true;