let QMPlay2 handle youtube:XXYYZZ URIs #619
Conversation
Committed from host : Bola
|
Figured out how to register the scheme on *nix . |
Committed from host : Bola
|
The (stupid) idea is that QMPlay2 doesn't know about YT, because it's an extension. But I'll check it soon! |
|
It's as you see fit, of course!
|
| // the leading '/' from the path to recover the intact vID. This implementation | ||
| // also supports youtube:/vID as a side effect ... why not! | ||
| auto vid = url.mid(8); | ||
| while (vid.at(0) == '/') |
There was a problem hiding this comment.
Crash if vid is empty string.
Btw. do we really need the youtube:/ (single slash) ? 😄
| } | ||
| return maybeExtensionAddress("http://www.youtube.com/watch?v=" + vid); | ||
| } | ||
| else |
There was a problem hiding this comment.
else not needed - previous block of code returns - no need to add indentation and else.
| @@ -2035,7 +2035,9 @@ bool MainWidget::eventFilter(QObject *obj, QEvent *event) | |||
| #ifdef Q_OS_MACOS | |||
| else if (event->type() == QEvent::FileOpen) | |||
| { | |||
| filesToAdd.append(((QFileOpenEvent *)event)->file()); | |||
| auto url = ((QFileOpenEvent *)event)->url(); | |||
There was a problem hiding this comment.
Could you add auto fileOpenEvent = (QFileOpenEvent *)event; and use it?
|
On Sunday July 30 2023 12:42:17 Błażej Szczygieł wrote:
Btw. do we really need the `youtube:/` (single slash) ? :smile:
The comment about answers 'no' to that question ;) You'd prefer to test for the presence of a `//` and remove that?
`else` not needed - previous block of code returns - no need to add indentation and `else`.
Indeed. Old habit ("compiler might be able to generate more efficient code"), plus I find it makes it easier to get reacquainted with the code flow if you haven't been in that area for a while.
|
My compiler (clang) generates the same code w/o I'd suggest (this should not trigger the undefined behavior and looks simpler + uses QString Functions::maybeExtensionAddress(const QString &url)
{
if (url.startsWith("youtube:"))
{
// we want to support both youtube:vID and youtube://vID but the latter form
// is often passed to us as "youtube://vid" (vID interpreted as the remote host).
// So we also support the full URI form with an empty host string and strip
// the leading '/' from the path to recover the intact vID. This implementation
// also supports youtube:/vID as a side effect ... why not!
const auto vid = url.mid(8).remove('/');
while (!vid.isEmpty())
return maybeExtensionAddress("https://www.youtube.com/watch?v=" + vid);
}
for (const QMPlay2Extensions *QMPlay2Ext : QMPlay2Extensions::QMPlay2ExtensionsList())
{
const QString prefix = QMPlay2Ext->matchAddress(url);
if (!prefix.isEmpty())
return prefix + "://{" + url + "}";
}
return url;
} |
|
Or this: diff --git a/src/modules/Extensions/YouTube.cpp b/src/modules/Extensions/YouTube.cpp
index d838ccb4..19e8dab4 100644
--- a/src/modules/Extensions/YouTube.cpp
+++ b/src/modules/Extensions/YouTube.cpp
@@ -436,7 +436,7 @@ bool YouTube::canConvertAddress() const
QString YouTube::matchAddress(const QString &url) const
{
const QUrl qurl(url);
- if (qurl.scheme().startsWith("http") && (qurl.host().contains("youtube.") || qurl.host().contains("youtu.be")))
+ if (qurl.scheme() == "youtube" || (qurl.scheme().startsWith("http") && (qurl.host().contains("youtube.") || qurl.host().contains("youtu.be"))))
return "YouTube";
if (qurl.scheme().startsWith("http") && qurl.host().contains("twitch.tv"))
return "youtube-dl";
@@ -460,7 +460,7 @@ void YouTube::convertAddress(const QString &prefix, const QString &url, const QS
if (ioCtrl && (stream_url || name))
{
auto &youTubeDl = ioCtrl->toRef<YouTubeDL>();
- const QStringList youTubeVideo = getYouTubeVideo(param, url, youTubeDl);
+ const QStringList youTubeVideo = getYouTubeVideo(param, url.startsWith("youtube:") ? url.mid(8).remove('/') : url, youTubeDl);
if (youTubeVideo.count() == 4)
{
if (stream_url)No need to modify QMPlay2, just the YT extension 😄 |
|
Btw. where |
|
Btw. where `youtube:` (without `//`) is needed?
Well, yes, that was in fact my original intention (think of the mailto: protocol). IIRC I've had to support the slashes because they could get added by the OS (probably the Mac's) but I cannot really remember.
|
On Linux ( |
|
Sorry, I wasn't clear: as per the issue title it was my intention to support the syntax _without_ slashes. You can argue that this is more appropriate too: "youtube" is not a service protocoI (think of it more as a hostnm as used with rsync) but there's also the concern to keep it simple for the user (me, for now ;)).
I'll have to check if I was obliged to add support for the syntax with the slashes or if that was a conscious choice.
|
Just keep the condition in |
|
Not changed according to my comments for over a year, closing. |
|
On Wednesday December 11 2024 08:34:26 Błażej Szczygieł wrote:
Not changed according to my comments for over a year, closing.
Fine with me - if I can't recall what those comments were (n)or why I didn't follow up :)
|
|
I don't remember, it's been 1.5 year 🙂 I also suggested more simple solution and got no feedback: #619 (comment) |
There are times where I want to refer to a YouTube video in shorthand notation that isn't usually recognised as "direct link" (e.g. on discussion forums where direct linking to videos with specific content isn't allowed). The obvious choice is
youtube:XXYYZZwhereXXYYZZis the video identifier.This patch enables QMPlay2 to process such URIs.
(This PR is a bit for fun and/or just-in-case)