iTunes: Share common import logic between XML and macOS

[fwcd]

This factors out the common, non-format-dependent logic between the iTunes importers into a new class ITunesDAO (feel free to suggest a better name), namely

• Database queries
• Duplicate resolution
• Playlist tree assembly

This means that the iTunes XML importer will now display the playlist folder hierarchy as a tree too!

Architecture

flowchart LR

ITunesFeature -->|calls| ITunesImporter
ITunesImporter -->ITunesXMLImporter
ITunesImporter --> ITunesMacOSImporter
ITunesXMLImporter -->|calls| ITunesDAO
ITunesMacOSImporter -->|calls| ITunesDAO

Loading

While the new architecture might seem a bit verbose, we get a cleaner separation of concerns and can reduce the platform-specific code, which especially in the case of the macOS importer may improve maintainability too.

[Swiftb0y]

This refactoring makes sense at first glance. I'll try to conduct a proper review in the near future. I'd suggest to call ITunesImportBackend ITunesPlaylistDAO or something since that seems to be its primary job, to be a Data Access Object.

[Swiftb0y]

In the meantime, can you fix the clazy warnings?

[fwcd]

I'd suggest to call ITunesImportBackend ITunesPlaylistDAO

In that case it should probably be ITunesDAO since it's responsible for tracks too, not just playlists.

[Swiftb0y]

Thank you, LGTM otherwise. I highly appreciate these refactoring PRs, even though they're probably in quite unused code.

[Swiftb0y]

I don't like the fact that these have to be virtual just in order to be tested. Is there no feature in gtest that can avoid that using some linker hacks?

[fwcd]

I've thought about this and the other option would be to use templates and parameterize the importer over the DAO. Unfortunately, that would also mean that we'd have to move the implementation into headers etc.

Since virtual methods are generally not thaat expensive (I've tested it on a library with hundreds of playlists and >10k tracks with pretty much no noticeable performance difference), I think this should be fine. The database calls are likely more an order of magnitude more expensive than the virtual function calls.

[Swiftb0y]

Right, in the scope of everything else, those virtual calls are probably going to be cheap, I'm just used to them being expensive in the context of cache latency and branch prediction.

[Swiftb0y]

LGTM, though I didn't conduct any manual tests