iTunes: Make playlist parser robust w.r.t <dict> key order
#11452
Conversation
| } else if (key == "Tracks") { | ||
| parseTracks(); | ||
| iTunesImport.playlistRoot = parsePlaylists(); | ||
| isTracksParsed = true; | ||
| } else if (key == "Playlists") { | ||
| iTunesImport.playlistRoot = parsePlaylists(); | ||
| } |
There was a problem hiding this comment.
I've just noticed that this might not even be sufficient.
While relying on the playlists being immediately located after the tracks in the XML (as we currently do) is brittle, we've traded this for another assumption: That inserting playlists into the db is safe, even if they happen to be located before the tracks in the XML. It seemed to work fine in my testing, but I'm not super familiar with how SQLite enforces such constraints around transactions either.
An alternative would be to parse everything in-memory first and then insert it into the db, the drawback would be that this might be very memory-intensive for large libraries. So I'm not sure what the best way to go about this would be.
There was a problem hiding this comment.
I see the problem. I'm no database nor parsing expert so I can't really judge the issue either. How big are these databases usually? I'd be surprised if they're so large that fitting them all in memory is infeasible given the capabilities of today's desktop computers.
There was a problem hiding this comment.
Most libraries I've seen range into the thousands/tens-of-thousands of tracks. My personal library has 8k tracks with a 21,7 MB XML file. You're probably right though that this shouldn't be a large memory burden for modern PCs.
There was a problem hiding this comment.
Given that the iTunes XML format is based on Apple's property list format, the 'right thing' to do would probably be to factor out an abstraction for parsing arbitrary plists on top of QXmlStreamReader or to use a library such as libplist (though introducing a new dependency for something as minor as this is likely not worth it).
fwcd
force-pushed
the
more-robust-itunes-playlist-parser
branch
from
30bcc50
to
849da57
Compare
|
This PR is marked as stale because it has been open 90 days with no activity. |
github-actions
bot
added
the
stale
github-actions
bot
removed
the
stale
|
This PR is marked as stale because it has been open 90 days with no activity. |
github-actions
bot
added
the
stale
Based on #11446, see here for a clean diff based on the other PR (rather than
2.4).The playlist parser is currently quite dependent on the order of the
<dict>keys, which in general isn't guaranteed in XML plists (similar to JSON). That iTunes/Apple Music always serializes the keys in a specific order is an implementation detail that we shouldn't rely on to parse playlists correctly.This PR fixes this by inserting the playlist and its tracks after the entire XML node has been parsed.