[DRAFT] resolve-from-urls #13146
[DRAFT] resolve-from-urls #13146
Conversation
…lper::supportedTracksFromUrls
|
Could this eventually lead to the possibility of pasting YouTube URLs into Mixxx to pull, convert and load audio into the next available deck? (an aspect of #6277) |
| QStringList pathList; | ||
| pathList.reserve(urls.size()); | ||
| for (const auto& url : urls) { | ||
| pathList << "(" + SqlStringFormatter::format(m_database, url.toLocalFile()) + ")"; |
There was a problem hiding this comment.
Can you add a source code comment about the braces?
There was a problem hiding this comment.
A minor issue is that because if that, the QString is allocated twice. Maybe we find a workaround for that.
There was a problem hiding this comment.
What happens if it is no local file? We should avoid "()"
There was a problem hiding this comment.
@daschuer do you have a fix in mind?
There was a problem hiding this comment.
What happens if it is no local file? We should avoid "()"
What about this? Like in DragAndDropHelper::supportedTracksFromUrls
| pathList << "(" + SqlStringFormatter::format(m_database, url.toLocalFile()) + ")"; | |
| QString urlStr = url.isLocalFile() ? url.toLocalFile() : url.toString() | |
| pathList << "(" + SqlStringFormatter::format(m_database, urlStr) + ")"; |
|
This is still draft, move form 2.5-beta milestone? |
I don't think so... |
It is draft because I find it the surrounding code hard to understand. The sole purpose of this change is to fix #13100 , and it looks to me that it does, but I find it hard to say if it is the right solution (also because @ronso0 suggested other approaches) and to predict if this can have negative side effects. I don't think we want to revert the tracklist shortkey handling for 2.5, as it is a very nice and very "announcable" improvement, but @ronso0 considers a #13100 a blocker (I think I agree), so it should be fixed. I am not quite sure how to proceed... Who would be the most familiar with the surrounding code? |
|
gonna take a in-detail look soonish. |
|
Well, it works as desired now, thanks! I also have a hard time double-checking all callers, and I didn't spent time on an alternative fix. |
|
I changed the base branch to 2.5, now that 2.6 has been split off. |
|
Yes, LGTM. @daschuer What do you think? |
|
This PR will allow to add any URL to the database regardless if it is a playable file or any other random address. I this the intended behavior? To reference streaming tracks for instance? |
|
@m0dB @daschuer How shall we proceed here? |
|
I consider this is as "ready for merge" with your fix in place. @m0dB will you have time to prepare that for merge? |
resolveTrackIdsFromUrls directly, without going through DragAndDropeHlper::supportedTracksFromUrls