-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support passing URLs as cmdline arguments #15
base: master
Are you sure you want to change the base?
Conversation
This makes it possible to use smtube with kokovp instead of smplayer.
ee8a8af
to
d3c9d48
Compare
Force-pushed to drop commits from other PR I accidentally included. |
@brainrom Could I please get your opinion on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of meaningless and confusing variable/methods renames, one mpv keyword is broken.
program_arg is over-engineering, which greatly limits class usage scope.
New URL converter is really needed and maybe will allow to accept URLs without any other changes.
@@ -188,7 +188,7 @@ void PlayerController::handleFileLoad() | |||
t.type = Track::TRACK_TYPE_SUB; | |||
|
|||
if (t.isExternal) | |||
t.filename = p->getProp(trackAddr + "external-filename").toString(); | |||
t.mediaUrl = p->getProp(trackAddr + "external-mediaUrl").toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wont work. external-filename
is the mpv's keyword.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
program_arg is over-engineering. SingleInstance is designed to be as simple as possible stream communication and doesn't require additional structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Massive rename without exact purpose. file can be accessed by url as well.
@@ -36,6 +37,7 @@ int main(int argc, char *argv[]) | |||
parser.addHelpOption(); | |||
parser.addVersionOption(); | |||
parser.addPositionalArgument("media", QCoreApplication::translate("KokoVP", "Media files to play")); | |||
parser.addPositionalArgument("urls", QCoreApplication::translate("KokoVP", "URLs to play")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How multiple position arguments lists should work at all? Maybe just accept media by local file paths and by urls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really useful for achieving accepting URLs. Needs testing and fixing mentioned issue, but anyway.
I've not renamed the vars with file in it in all files as I want your opinion on that 1st. I settled for
mediaUrl
andmediaRessource
, to make it more obvious that it may also be a stream and "medium" sounded more too much like esoteric magic.Spaces in URLs don't work yet.