Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Added embedded CUE sheet support

  • Loading branch information...
commit 6214263a6b0cea8d92ebb920f0702cb252afb6d9 1 parent 303712d
psyton authored
View
188 xbmc/CueDocument.cpp
@@ -62,13 +62,93 @@
#include "filesystem/Directory.h"
#include "FileItem.h"
#include "settings/AdvancedSettings.h"
+#include "filesystem/File.h"
+#include "music/tags/MusicInfoTag.h"
#include <set>
using namespace std;
using namespace XFILE;
-CCueDocument::CCueDocument(void)
+class FileCueReader
+ : public CueReader
+{
+ CFile m_file;
+ bool m_opened;
+ char m_szBuffer[1024];
+public:
+ FileCueReader(const CStdString &strFile)
+ {
+ m_opened = m_file.Open(strFile);
+ }
+ virtual bool ReadNextLine(CStdString &szLine)
+ {
+ char *pos;
+ // Read the next line.
+ while (m_file.ReadString(m_szBuffer, 1023)) // Bigger than MAX_PATH_SIZE, for usage with relax!
+ {
+ // Remove the white space at the beginning of the line.
+ pos = m_szBuffer;
+ while (pos && skipChar(*pos)) pos++;
+ if (pos)
+ {
+ szLine = pos;
+ return true;
+ }
+ // If we are here, we have an empty line so try the next line
+ }
+ return false;
+ }
+ ~FileCueReader()
@PSyton Owner
PSyton added a note

it improves readability, but it is not necessary. Ok, i will ad it to final code. In next commit i've refactored this code.

@PSyton Owner
PSyton added a note

Base class is virtual and it derived classes we can drop out 'virtual' keyword. But we write it for better understandig.
In later commits which I've made later done massive rafactoring and added 'virtual'.

@PSyton Owner
PSyton added a note

Ok. I've already fixed this d565dad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {
+ if (m_opened)
+ m_file.Close();
+ }
+};
+
+class TagCueReader
+ : public CueReader
+{
+ CStdString m_data;
+ unsigned int m_currentPos;
+public:
+ TagCueReader(const CStdString &cueData)
+ : m_data(cueData)
+ , m_currentPos(0)
+ {
+ }
+ virtual bool ReadNextLine(CStdString &line)
+ {
+ // Read the next line.
+ line = "";

line.clear() is normally better - no insistence though.

@PSyton Owner
PSyton added a note

Ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ bool stop = false;
+ while (m_currentPos < m_data.length())
+ {
+ // Remove the white space at the beginning of the line.
+ char ch = m_data.at(m_currentPos++);
+ stop |= (!skipChar(ch));
+ if (stop)
+ {
+ if (ch == '\r' || ch == '\n')
+ {
+ if (!line.IsEmpty())
+ return true;
+ }
+ else
+ line += ch;
+ }
+ }
+ return false;
+ }
+ ~TagCueReader()
+ {
+ }
+};
+
+
+CCueDocument::CCueDocument(const CFileItem &fileItem)
+ : m_external(true)
+ , m_reader(0)
{
m_strArtist = "";
m_strAlbum = "";
@@ -78,20 +158,65 @@ CCueDocument::CCueDocument(void)
m_replayGainAlbumGain = 0.0f;
m_iTotalTracks = 0;
m_iTrack = 0;
+ m_sourcePath = fileItem.GetPath();
+ m_valid = Load(fileItem);
}
-CCueDocument::~CCueDocument(void)
-{}
+CCueDocument::~CCueDocument()
+{
+ delete m_reader;
+}
+
+bool CCueDocument::isValid() const
+{
+ return m_valid;
+}
+
+bool CCueDocument::isExternal() const
+{
+ return m_external;
+}
+
+
+////////////////////////////////////////////////////////////////////////////////////
+// Function: Load()
+// Try to load cuesheet information from file.
+////////////////////////////////////////////////////////////////////////////////////
+bool CCueDocument::Load(const CFileItem &fileItem)
+{
+ if (fileItem.IsCUESheet()) // *.CUE - file
+ {
+ m_reader = new FileCueReader(fileItem.GetPath());
+ m_external = true;
+ }
+ else
+ {
+ CStdString extension;
+ URIUtils::GetExtension(fileItem.GetPath(), extension);
+ if (extension.Equals((char*)".ape") ||
+ extension.Equals((char*)".flac") ||
+ extension.Equals((char*)".wv") ||
+ extension.Equals((char*)".ogg"))
+ {
+ MUSIC_INFO::CMusicInfoTag tag(fileItem.GetPath());
+ if (tag.hasEmbeddedCue())
+ {
+ m_reader = new TagCueReader(tag.GetEmbeddedCue());
+ m_external = false;
+ }
+ }

This should load the tag from the file only if we need to (i.e. check whether the fileItem already has the tag loaded). If it doesn't have the tag loaded, we should only read it if tag reading is enabled.

ATM you've just made a large directory of flacs load very slowly in the hope that one of them might contain a cue sheet.

@PSyton Owner
PSyton added a note

Later I've added cache for scanned files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }
+ return Parse();
+}
////////////////////////////////////////////////////////////////////////////////////
// Function: Parse()
// Opens the .cue file for reading, and constructs the track database information
////////////////////////////////////////////////////////////////////////////////////
-bool CCueDocument::Parse(const CStdString &strFile)
+bool CCueDocument::Parse()
{
- if (!m_file.Open(strFile))
+ if (!m_reader)
return false;
-
CStdString strLine;
m_iTotalTracks = -1;
CStdString strCurrentFile = "";
@@ -101,7 +226,7 @@ bool CCueDocument::Parse(const CStdString &strFile)
// Run through the .CUE file and extract the tracks...
while (true)
{
- if (!ReadNextLine(strLine))
+ if (!m_reader->ReadNextLine(strLine))
break;
if (strLine.Left(7) == "INDEX 0")
{
@@ -155,7 +280,7 @@ bool CCueDocument::Parse(const CStdString &strFile)
CCueTrack track;
m_Track.push_back(track);
- m_Track[m_iTotalTracks].strFile = strCurrentFile;
+ m_Track[m_iTotalTracks].strFile = isExternal() ? strCurrentFile : m_sourcePath;
if (iTrackNumber > 0)
m_Track[m_iTotalTracks].iTrackNumber = iTrackNumber;
@@ -166,15 +291,18 @@ bool CCueDocument::Parse(const CStdString &strFile)
}
else if (strLine.Left(4) == "FILE")
{
- // already a file name? then the time computation will be changed
- if(strCurrentFile.size() > 0)
- bCurrentFileChanged = true;
+ if (isExternal())
+ {
+ // already a file name? then the time computation will be changed
+ if(strCurrentFile.size() > 0)
+ bCurrentFileChanged = true;
- ExtractQuoteInfo(strLine, strCurrentFile);
+ ExtractQuoteInfo(strLine, strCurrentFile);
- // Resolve absolute paths (if needed).
- if (strCurrentFile.length() > 0)
- ResolvePath(strCurrentFile, strFile);
+ // Resolve absolute paths (if needed).
+ if (strCurrentFile.length() > 0)
+ ResolvePath(strCurrentFile);
+ }
}
else if (strLine.Left(8) == "REM DATE")
{
@@ -211,7 +339,7 @@ bool CCueDocument::Parse(const CStdString &strFile)
m_Track[m_iTotalTracks].iEndTime = 0;
else
OutputDebugString("No INDEX 01 tags in CUE file!\n");
- m_file.Close();
+
if (m_iTotalTracks >= 0)
{
m_iTotalTracks++;
@@ -271,30 +399,6 @@ CStdString CCueDocument::GetMediaTitle()
// Private Functions start here
////////////////////////////////////////////////////////////////////////////////////
-// Function: ReadNextLine()
-// Returns the next non-blank line of the textfile, stripping any whitespace from
-// the left.
-////////////////////////////////////////////////////////////////////////////////////
-bool CCueDocument::ReadNextLine(CStdString &szLine)
-{
- char *pos;
- // Read the next line.
- while (m_file.ReadString(m_szBuffer, 1023)) // Bigger than MAX_PATH_SIZE, for usage with relax!
- {
- // Remove the white space at the beginning of the line.
- pos = m_szBuffer;
- while (pos && (*pos == ' ' || *pos == '\t' || *pos == '\r' || *pos == '\n')) pos++;
- if (pos)
- {
- szLine = pos;
- return true;
- }
- // If we are here, we have an empty line so try the next line
- }
- return false;
-}
-
-////////////////////////////////////////////////////////////////////////////////////
// Function: ExtractQuoteInfo()
// Extracts the information in quotes from the string line, returning it in quote
////////////////////////////////////////////////////////////////////////////////////
@@ -360,10 +464,10 @@ int CCueDocument::ExtractNumericInfo(const CStdString &info)
// Determines whether strPath is a relative path or not, and if so, converts it to an
// absolute path using the path information in strBase
////////////////////////////////////////////////////////////////////////////////////
-bool CCueDocument::ResolvePath(CStdString &strPath, const CStdString &strBase)
+bool CCueDocument::ResolvePath(CStdString &strPath)
{
CStdString strDirectory;
- URIUtils::GetDirectory(strBase, strDirectory);
+ URIUtils::GetDirectory(m_sourcePath, strDirectory);
CStdString strFilename = strPath;
URIUtils::GetFileName(strFilename);
View
45 xbmc/CueDocument.h
@@ -22,10 +22,27 @@
*/
#include "music/Song.h"
-#include "filesystem/File.h"
#define MAX_PATH_SIZE 1024
+namespace XFILE {
+ class CFile;
+}
+
+class CFileItem;
+class CueReader
+{
+ bool m_external;
+public:
+ CueReader() {}
+ inline bool skipChar(char ch) const
+ {
+ return (ch == ' ' || ch == '\t' || ch == '\r' || ch == '\n');
+ }
+ virtual bool ReadNextLine(CStdString &line) = 0;
+ virtual ~CueReader() {};
+};
+
class CCueDocument
{
class CCueTrack
@@ -50,21 +67,18 @@ class CCueDocument
};
public:
- CCueDocument(void);
- ~CCueDocument(void);
+ CCueDocument(const CFileItem &fileItem);
+ ~CCueDocument();
// USED
- bool Parse(const CStdString &strFile);
+
+ bool isValid() const;
+ bool isExternal() const;
void GetSongs(VECSONGS &songs);
CStdString GetMediaPath();
CStdString GetMediaTitle();
void GetMediaFiles(std::vector<CStdString>& mediaFiles);
private:
-
- // USED for file access
- XFILE::CFile m_file;
- char m_szBuffer[1024];
-
// Member variables
CStdString m_strArtist; // album artist
CStdString m_strAlbum; // album title
@@ -74,13 +88,22 @@ class CCueDocument
int m_iTotalTracks; // total tracks
float m_replayGainAlbumGain;
float m_replayGainAlbumPeak;
+ bool m_valid;
+ bool m_external;
+ CStdString m_sourcePath;
+
+ CueReader* m_reader;
// cuetrack array
std::vector<CCueTrack> m_Track;
- bool ReadNextLine(CStdString &strLine);
+ bool Parse();
+
+ //bool ReadNextLine(XFILE::CFile& file, CStdString &strLine);
bool ExtractQuoteInfo(const CStdString &line, CStdString &quote);
int ExtractTimeFromIndex(const CStdString &index);
int ExtractNumericInfo(const CStdString &info);
- bool ResolvePath(CStdString &strPath, const CStdString &strBase);
+ bool ResolvePath(CStdString &strPath);
+
+ bool Load(const CFileItem &fileItem);
};
View
175 xbmc/FileItem.cpp
@@ -29,6 +29,7 @@
#include "utils/Crc32.h"
#include "filesystem/DirectoryCache.h"
#include "filesystem/StackDirectory.h"
+#include "filesystem/File.h"
#include "filesystem/CurlFile.h"
#include "filesystem/MultiPathDirectory.h"
#include "filesystem/MusicDatabaseDirectory.h"
@@ -94,7 +95,7 @@ CFileItem::CFileItem(const CStdString &path, const CAlbum& album)
else
m_strThumbnailImage.clear();
m_bIsAlbum = true;
- CMusicDatabase::SetPropertiesFromAlbum(*this,album);
+ CMusicDatabase::SetPropertiesFromAlbum(*this, album);
}
CFileItem::CFileItem(const CMusicInfoTag& music)
@@ -1837,119 +1838,107 @@ int CFileItemList::GetSelectedCount() const
void CFileItemList::FilterCueItems()
{
CSingleLock lock(m_lock);
- // Handle .CUE sheet files...
+ // Handle .CUE sheet files and media with embedded CUE sheet
VECSONGS itemstoadd;
CStdStringArray itemstodelete;
for (int i = 0; i < (int)m_items.size(); i++)
{
CFileItemPtr pItem = m_items[i];
- if (!pItem->m_bIsFolder)
- { // see if it's a .CUE sheet
- if (pItem->IsCUESheet())
+ if (pItem->m_bIsFolder)
+ continue;
+ // see if it has CUE sheet
+ CCueDocument cuesheet(*pItem);
+ if (cuesheet.isValid())
+ {
+ std::vector<CStdString> MediaFileVec;
+ cuesheet.GetMediaFiles(MediaFileVec);
+
+ // queue the cue sheet and the underlying media file for deletion
+ for(std::vector<CStdString>::iterator itMedia = MediaFileVec.begin(); itMedia != MediaFileVec.end(); itMedia++)
{
- CCueDocument cuesheet;
- if (cuesheet.Parse(pItem->GetPath()))
+ CStdString strMediaFile = *itMedia;
+ CStdString fileFromCue = strMediaFile; // save the file from the cue we're matching against,
+ // as we're going to search for others here...
+ bool bFoundMediaFile = CFile::Exists(strMediaFile);
+ // queue the cue sheet and the underlying media file for deletion
+ if (!bFoundMediaFile)
{
- VECSONGS newitems;
- cuesheet.GetSongs(newitems);
-
- std::vector<CStdString> MediaFileVec;
- cuesheet.GetMediaFiles(MediaFileVec);
-
- // queue the cue sheet and the underlying media file for deletion
- for(std::vector<CStdString>::iterator itMedia = MediaFileVec.begin(); itMedia != MediaFileVec.end(); itMedia++)
+ // try file in same dir, not matching case...
+ if (Contains(strMediaFile))
{
- CStdString strMediaFile = *itMedia;
- CStdString fileFromCue = strMediaFile; // save the file from the cue we're matching against,
- // as we're going to search for others here...
- bool bFoundMediaFile = CFile::Exists(strMediaFile);
- // queue the cue sheet and the underlying media file for deletion
- if (!bFoundMediaFile)
+ bFoundMediaFile = true;
+ }
+ else
+ {
+ // try removing the .cue extension...
+ strMediaFile = pItem->GetPath();
+ URIUtils::RemoveExtension(strMediaFile);
+ CFileItem item(strMediaFile, false);
+ if (item.IsAudio() && Contains(strMediaFile))
{
- // try file in same dir, not matching case...
- if (Contains(strMediaFile))
- {
- bFoundMediaFile = true;
- }
- else
+ bFoundMediaFile = true;
+ }
+ else
+ { // try replacing the extension with one of our allowed ones.
+ CStdStringArray extensions;
+ StringUtils::SplitString(g_settings.m_musicExtensions, "|", extensions);
+ for (unsigned int i = 0; i < extensions.size(); i++)
{
- // try removing the .cue extension...
- strMediaFile = pItem->GetPath();
- URIUtils::RemoveExtension(strMediaFile);
+ strMediaFile = URIUtils::ReplaceExtension(pItem->GetPath(), extensions[i]);
CFileItem item(strMediaFile, false);
- if (item.IsAudio() && Contains(strMediaFile))
+ if (!item.IsCUESheet() && !item.IsPlayList() && Contains(strMediaFile))
{
bFoundMediaFile = true;
- }
- else
- { // try replacing the extension with one of our allowed ones.
- CStdStringArray extensions;
- StringUtils::SplitString(g_settings.m_musicExtensions, "|", extensions);
- for (unsigned int i = 0; i < extensions.size(); i++)
- {
- strMediaFile = URIUtils::ReplaceExtension(pItem->GetPath(), extensions[i]);
- CFileItem item(strMediaFile, false);
- if (!item.IsCUESheet() && !item.IsPlayList() && Contains(strMediaFile))
- {
- bFoundMediaFile = true;
- break;
- }
- }
+ break;
}
}
}
- if (bFoundMediaFile)
+ }
+ }
+ if (bFoundMediaFile)
+ {
+ // get the additional stuff (year, genre etc.) from the underlying media files tag.
+ CMusicInfoTag tag(strMediaFile);
+
+ // Skip external .CUE if media contains embedded CUE
+ if (cuesheet.isExternal() && tag.hasEmbeddedCue())
+ continue;
+
+ VECSONGS newitems;
+ cuesheet.GetSongs(newitems);
+
+ // fill in any missing entries from underlying media file
+ for (int j = 0; j < (int)newitems.size(); j++)
+ {
+ CSong song = newitems[j];
+ // only for songs that actually match the current media file
+ if (song.strFileName == fileFromCue)
{
- itemstodelete.push_back(pItem->GetPath());
- itemstodelete.push_back(strMediaFile);
- // get the additional stuff (year, genre etc.) from the underlying media files tag.
- CMusicInfoTag tag;
- auto_ptr<IMusicInfoTagLoader> pLoader (CMusicInfoTagLoaderFactory::CreateLoader(strMediaFile));
- if (NULL != pLoader.get())
+ // we might have a new media file from the above matching code
+ song.strFileName = strMediaFile;
+ if (tag.Loaded())
{
- // get id3tag
- pLoader->Load(strMediaFile, tag);
+ if (song.strAlbum.empty() && !tag.GetAlbum().empty()) song.strAlbum = tag.GetAlbum();
+ if (song.albumArtist.empty() && !tag.GetAlbumArtist().empty()) song.albumArtist = tag.GetAlbumArtist();
+ if (song.genre.empty() && !tag.GetGenre().empty()) song.genre = tag.GetGenre();
+ if (song.artist.empty() && !tag.GetArtist().empty()) song.artist = tag.GetArtist();
+ if (tag.GetDiscNumber()) song.iTrack |= (tag.GetDiscNumber() << 16); // see CMusicInfoTag::GetDiscNumber()
+ SYSTEMTIME dateTime;
+ tag.GetReleaseDate(dateTime);
+ if (dateTime.wYear) song.iYear = dateTime.wYear;
}
- // fill in any missing entries from underlying media file
- for (int j = 0; j < (int)newitems.size(); j++)
- {
- CSong song = newitems[j];
- // only for songs that actually match the current media file
- if (song.strFileName == fileFromCue)
- {
- // we might have a new media file from the above matching code
- song.strFileName = strMediaFile;
- if (tag.Loaded())
- {
- if (song.strAlbum.empty() && !tag.GetAlbum().empty()) song.strAlbum = tag.GetAlbum();
- if (song.albumArtist.empty() && !tag.GetAlbumArtist().empty()) song.albumArtist = tag.GetAlbumArtist();
- if (song.genre.empty() && !tag.GetGenre().empty()) song.genre = tag.GetGenre();
- if (song.artist.empty() && !tag.GetArtist().empty()) song.artist = tag.GetArtist();
- if (tag.GetDiscNumber()) song.iTrack |= (tag.GetDiscNumber() << 16); // see CMusicInfoTag::GetDiscNumber()
- SYSTEMTIME dateTime;
- tag.GetReleaseDate(dateTime);
- if (dateTime.wYear) song.iYear = dateTime.wYear;
- }
- if (!song.iDuration && tag.GetDuration() > 0)
- { // must be the last song
- song.iDuration = (tag.GetDuration() * 75 - song.iStartOffset + 37) / 75;
- }
- // add this item to the list
- itemstoadd.push_back(song);
- }
+ if (!song.iDuration && tag.GetDuration() > 0)
+ { // must be the last song
+ song.iDuration = (tag.GetDuration() * 75 - song.iStartOffset + 37) / 75;
}
- }
- else
- { // remove the .cue sheet from the directory
- itemstodelete.push_back(pItem->GetPath());
+ // add this item to the list
+ itemstoadd.push_back(song);
}
}
}
- else
- { // remove the .cue sheet from the directory (can't parse it - no point listing it)
- itemstodelete.push_back(pItem->GetPath());
- }
}
+ // Always remove .CUE file from the directory
+ itemstodelete.push_back(pItem->GetPath());
}
}
// now delete the .CUE files and underlying media files.
@@ -3010,6 +2999,12 @@ MUSIC_INFO::CMusicInfoTag* CFileItem::GetMusicInfoTag()
if (!m_musicInfoTag)
m_musicInfoTag = new MUSIC_INFO::CMusicInfoTag;
+ if (!m_musicInfoTag->Loaded())
+ { // read the tag from a file
+ auto_ptr<IMusicInfoTagLoader> pLoader (CMusicInfoTagLoaderFactory::CreateLoader(GetPath()));
+ if (NULL != pLoader.get())
+ pLoader->Load(GetPath(), *m_musicInfoTag);
+ }

No - we don't want to attempt to load the tag every time we Get() it.

@PSyton Owner
PSyton added a note

But it's a GetMusicInfoTag()! - when it's called - we need the tag if it exists.
Why need we check for loading and load (if it's not done yet) every time when we call GetMusicInfoTag()?

Because most of the time you don't want to read the tag from the file. We might want to fill it from the database for example.. The small number of places where you actually want to read it from a file can be handled by reading it there and then.

I have no problem with helpers for this purpose should you feel it's needed (eg there's one in CFileItem IIRC).

@PSyton Owner
PSyton added a note

Ok. I agreed.
Look at this c5d32c3
May be it's more clean and DRY.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
return m_musicInfoTag;
}
View
6 xbmc/music/infoscanner/MusicInfoScanner.cpp
@@ -479,12 +479,6 @@ int CMusicInfoScanner::RetrieveMusicInfo(CFileItemList& items, const CStdString&
CSong *dbSong = songsMap.Find(pItem->GetPath());
CMusicInfoTag& tag = *pItem->GetMusicInfoTag();
- if (!tag.Loaded() )
- { // read the tag from a file
- auto_ptr<IMusicInfoTagLoader> pLoader (CMusicInfoTagLoaderFactory::CreateLoader(pItem->GetPath()));
- if (NULL != pLoader.get())
- pLoader->Load(pItem->GetPath(), tag);
- }
// if we have the itemcount, notify our
// observer with the progress we made
View
3  xbmc/music/tags/APEv2Tag.cpp
@@ -128,6 +128,9 @@ bool CAPEv2Tag::ReadTag(const char* filename)
m_rating = temp;
}
+ if (apefrm_getstr(tag, (char*)"Cuesheet"))
+ m_cueSheet = apefrm_getstr(tag, (char*)"Cuesheet");
+
// Replay gain info
GetReplayGainFromTag(tag);
View
2  xbmc/music/tags/APEv2Tag.h
@@ -51,6 +51,7 @@ class CAPEv2Tag
CStdString GetMusicBrainzTRMID() { return m_strMusicBrainzTRMID; }
CStdString GetComment() { return m_strComment; };
CStdString GetLyrics() { return m_strLyrics; };
+ const CStdString& GetEmbeddedCue() const { return m_cueSheet; };
char GetRating() { return m_rating; };
void GetReplayGainFromTag(apetag *tag);
const CReplayGain &GetReplayGain() { return m_replayGain; };
@@ -78,6 +79,7 @@ class CAPEv2Tag
CReplayGain m_replayGain;
int64_t m_nDuration;
char m_rating;
+ CStdString m_cueSheet;
DllLibApeTag m_dll;
};
View
28 xbmc/music/tags/MusicInfoTag.cpp
@@ -24,12 +24,20 @@
#include "utils/StringUtils.h"
#include "settings/AdvancedSettings.h"
#include "utils/Variant.h"
+#include "filesystem\File.h"
+#include "music/tags/MusicInfoTagLoaderFactory.h"
using namespace MUSIC_INFO;
-CMusicInfoTag::CMusicInfoTag(void)
+CMusicInfoTag::CMusicInfoTag(const CStdString& strMediaFile /*= CStdString()*/)
{
Clear();
+ if (XFILE::CFile::Exists(strMediaFile))
+ {
+ std::auto_ptr<IMusicInfoTagLoader> pLoader (CMusicInfoTagLoaderFactory::CreateLoader(strMediaFile));
+ if (NULL != pLoader.get())
+ pLoader->Load(strMediaFile, *this);
+ }

Please no - you've just done a stat for "" every time you create a CMusicInfoTag. The CMusicInfoTag should not need to know about the loader IMO - put this where it needs to be instead.

@PSyton Owner
PSyton added a note

I can add test for "".
But if i revert this to original, I need write 5 lines of code every time where I need tag:

if (!tag.Loaded() )
{ // read the tag from a file
  auto_ptr<IMusicInfoTagLoader> pLoader (CMusicInfoTagLoaderFactory::CreateLoader(pItem->GetPath()));
  if (NULL != pLoader.get())
    pLoader->Load(pItem->GetPath(), tag);
}

it's not good idea too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
CMusicInfoTag::CMusicInfoTag(const CMusicInfoTag& tag)
@@ -67,6 +75,7 @@ const CMusicInfoTag& CMusicInfoTag::operator =(const CMusicInfoTag& tag)
m_iDbId = tag.m_iDbId;
m_iArtistId = tag.m_iArtistId;
m_iAlbumId = tag.m_iAlbumId;
+ m_strCue = tag.m_strCue;
memcpy(&m_dwReleaseDate, &tag.m_dwReleaseDate, sizeof(m_dwReleaseDate) );
return *this;
}
@@ -421,6 +430,22 @@ void CMusicInfoTag::SetSong(const CSong& song)
m_iAlbumId = song.iAlbumId;
}
+bool CMusicInfoTag::hasEmbeddedCue() const
@PSyton Owner
PSyton added a note

Ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+{
+ return !GetEmbeddedCue().IsEmpty();
+}
+
+const CStdString& CMusicInfoTag::GetEmbeddedCue() const
+{
+ return m_strCue;
+}
+
+void CMusicInfoTag::setEmbeddedCue(const CStdString& cuesheet)
+{
+ m_strCue = cuesheet;
+}
+
+
void CMusicInfoTag::Serialize(CVariant& value)
{
/* TODO:
@@ -519,6 +544,7 @@ void CMusicInfoTag::Clear()
m_bLoaded = false;
m_lastPlayed.Reset();
m_strComment.Empty();
+ m_strCue.Empty();
m_rating = '0';
m_iDbId = -1;
m_iTimesPlayed = 0;
View
7 xbmc/music/tags/MusicInfoTag.h
@@ -34,7 +34,7 @@ namespace MUSIC_INFO
class CMusicInfoTag : public IArchivable, public ISerializable
{
public:
- CMusicInfoTag(void);
+ CMusicInfoTag(const CStdString& strMediaFile = CStdString());
CMusicInfoTag(const CMusicInfoTag& tag);
virtual ~CMusicInfoTag();
const CMusicInfoTag& operator =(const CMusicInfoTag& tag);
@@ -119,6 +119,10 @@ class CMusicInfoTag : public IArchivable, public ISerializable
*/
void AppendGenre(const CStdString &genre);
+ bool hasEmbeddedCue() const;
+ const CStdString& GetEmbeddedCue() const;
+ void setEmbeddedCue(const CStdString& cuesheet);
+
virtual void Archive(CArchive& ar);
virtual void Serialize(CVariant& ar);
@@ -143,6 +147,7 @@ class CMusicInfoTag : public IArchivable, public ISerializable
CStdString m_strMusicBrainzTRMID;
CStdString m_strComment;
CStdString m_strLyrics;
+ CStdString m_strCue;
CDateTime m_lastPlayed;
int m_iDuration;
int m_iTrack; // consists of the disk number in the high 16 bits, the track number in the low 16bits
View
1  xbmc/music/tags/MusicInfoTagLoaderApe.cpp
@@ -63,6 +63,7 @@ bool CMusicInfoTagLoaderApe::Load(const CStdString& strFileName, CMusicInfoTag&
tag.SetMusicBrainzArtistID(myTag.GetMusicBrainzArtistID());
tag.SetMusicBrainzTrackID(myTag.GetMusicBrainzTrackID());
tag.SetMusicBrainzTRMID(myTag.GetMusicBrainzTRMID());
+ tag.setEmbeddedCue(myTag.GetEmbeddedCue());
SYSTEMTIME dateTime;
ZeroMemory(&dateTime, sizeof(SYSTEMTIME));
dateTime.wYear = atoi(myTag.GetYear());
View
2  xbmc/music/tags/MusicInfoTagLoaderMP3.cpp
@@ -132,6 +132,8 @@ bool CMusicInfoTagLoaderMP3::Load(const CStdString& strFileName, CMusicInfoTag&
tag.SetComment(apeTag.GetComment());
if (apeTag.GetReplayGain().iHasGainInfo)
m_replayGainInfo = apeTag.GetReplayGain();
+ if (apeTag.GetEmbeddedCue().size())
+ tag.setEmbeddedCue(apeTag.GetEmbeddedCue());
if (apeTag.GetRating() > '0')
tag.SetRating(apeTag.GetRating());
}
View
5 xbmc/music/tags/VorbisTag.cpp
@@ -126,6 +126,11 @@ int CVorbisTag::ParseTagEntry(CStdString& strTagEntry)
if ( strTagType == "RATING" && strTagValue.GetLength() == 1 && strTagValue[0] > '0' && strTagValue[0] < '6')
tag.SetRating(strTagValue[0]);
+ if (strTagType=="CUESHEET")
+ {
+ tag.setEmbeddedCue(strTagValue);
+ }
+
// Get new style replay gain info
if (strTagType=="REPLAYGAIN_TRACK_GAIN")
{
@PSyton

it improves readability, but it is not necessary. Ok, i will ad it to final code. In next commit i've refactored this code.

@PSyton

Base class is virtual and it derived classes we can drop out 'virtual' keyword. But we write it for better understandig.
In later commits which I've made later done massive rafactoring and added 'virtual'.

@PSyton

Ok. I've already fixed this d565dad

@jmarshallnz

line.clear() is normally better - no insistence though.

@jmarshallnz

Please no - you've just done a stat for "" every time you create a CMusicInfoTag. The CMusicInfoTag should not need to know about the loader IMO - put this where it needs to be instead.

@jmarshallnz

No - we don't want to attempt to load the tag every time we Get() it.

@jmarshallnz

This should load the tag from the file only if we need to (i.e. check whether the fileItem already has the tag loaded). If it doesn't have the tag loaded, we should only read it if tag reading is enabled.

ATM you've just made a large directory of flacs load very slowly in the hope that one of them might contain a cue sheet.

@PSyton

I can add test for "".
But if i revert this to original, I need write 5 lines of code every time where I need tag:

if (!tag.Loaded() )
{ // read the tag from a file
  auto_ptr<IMusicInfoTagLoader> pLoader (CMusicInfoTagLoaderFactory::CreateLoader(pItem->GetPath()));
  if (NULL != pLoader.get())
    pLoader->Load(pItem->GetPath(), tag);
}

it's not good idea too.

@PSyton

Later I've added cache for scanned files.

@PSyton

But it's a GetMusicInfoTag()! - when it's called - we need the tag if it exists.
Why need we check for loading and load (if it's not done yet) every time when we call GetMusicInfoTag()?

@jmarshallnz

Because most of the time you don't want to read the tag from the file. We might want to fill it from the database for example.. The small number of places where you actually want to read it from a file can be handled by reading it there and then.

I have no problem with helpers for this purpose should you feel it's needed (eg there's one in CFileItem IIRC).

@PSyton

Ok. I agreed.
Look at this c5d32c3
May be it's more clean and DRY.

Please sign in to comment.
Something went wrong with that request. Please try again.