Implement playlist and crate file export.#849
Conversation
This copies all of the tracks in a crate or playlist into a folder, such as an external USB stick. Only files are copied, not mixxx-specific metadata like waveforms or cover art that is not embedded in the file.
|
Neat! High level comments:
|
|
Once you have a size estimate, how involved would it be to check to see if the destination file system has enough space? And don't even start the copy if not. (Partial multiple-file copies are a pain to fix so let's avoid them.) |
There was a problem hiding this comment.
Since this is new code, please factor it out now.
There was a problem hiding this comment.
what's a good place to put factored-out code like this?
There was a problem hiding this comment.
util/soundfileexport.cpp seems sensible
There was a problem hiding this comment.
Or since this is specific to the library, just put it in library/soundfileexport.cpp
There was a problem hiding this comment.
what's a good place to put factored-out code like this?
I think you're going to end up needing a few different components -- a dialog, a thread for doing the work in, some utilities for serializing the Mixxx metadata (once you get to that) and such so maybe library/export/ is a reasonable place to stick code related to this? The LibraryScanner is a similar model.
There was a problem hiding this comment.
(probably don't need a threadpool / task thing like the LibraryScanner does though)
|
ah yes progress bar will be much nicer than the stupid messagebox I have at the end. Size estimate is not too hard, but getting remaining disk space is tricky. I'd prefer to just run out of space and have the resulting error pop up. |
|
closing for now while I do a major refactor |
|
reopened! Massive reworking to make it interactive and non-blocking (although it is modal). still without official tests, though I will try to write some. |
There was a problem hiding this comment.
actually I'm going to have the dialog take a list of TrackPointers instead of locations. In the future we may want to add more sophistication to the process, like make sure id3 tags are synced for each file, and even transcoding files on output. CDJs don't support flac, so if the user (e.g. me :)) wants to use their tracks on a CDJ, they'll need to convert FLACs to AIFF or WAV. By passing TrackPointers, we have a lot more information about the items being exported.
|
Nice feature! It would be cool if we add a metadata file that contains the cue points and other things that we have in Mixxx database and allow to import it by an other Mixxx instance. |
Reformat code a bit
|
My sense is that your comment is less about an actual problem or shortcoming or bug with the design, and more just that if you'd done it yourself you'd do it differently. Unless you (or any one else) feels strongly about the ownership or gui/controller split, I'd prefer not to do that for now. When I revisit this feature to do file conversion or metadata export we can refactor the architecture as needed. |
There was a problem hiding this comment.
This has the same "could have an active search" issue -- did you want to make a new table model here?
There was a problem hiding this comment.
I'm not sure how to write this -- BasePlaylistFeature is a base class and I don't know how to say "make a copy of the implementation class". TODO
There was a problem hiding this comment.
Both PlaylistFeature and SetlogFeature use a PlaylistTableModel so you don't have to worry about that -- see slotExportPlaylist above which does the same sort of thing.
|
Overall usability wise I wonder what the likelihood is that two tracks have the same filename in a typical user collection. Like what if you have two album directories and each one is like "01.mp3" -- that's fairly common, right? It would make the export feature kind of annoying to use since your only choices are a) overwrite one at "random" b) rename the file on disk and then get mIxxx to recognize that change and then re-export c) remove the file from the playlist/crate and re-export? We know all the files that the user wants to export so we can predict collisions like that and prepend the directory name or something? Anyway, I don't see a strong need to deal with it in this PR, just idly wondering how common this complaint will be. |
|
Overall the code looks like it's in good shape to me. I'm glad the std::future approach seems to work. It's a shame QFuture is so crappy :-/. thanks for the useful feature! |
|
I'd like to make playlist entry deduping and file renaming a TODO for a subsequent PR, with the understanding that we don't want to release until those bugs are fixed. |
Various other review notes
|
notes addressed. I still have to write a test, but I did a basic export by hand and I didn't obviously break anything |
SGTM - this just gets the stake in the ground. |
|
lgtm, thanks! |
Implement playlist and crate file export.
|
Thanks for starting work on this! :) Seems that a lot of users have been asking for this feature lately. |
6 participants
This copies all of the tracks in a crate or playlist into a folder such as an external USB stick. Only files are copied, not mixxx-specific metadata like waveforms or cover art that is not embedded in the file.