Review Request: So master isn't frozen anymore...
Nikita Skovoroda
chalkerx at gmail.com
Sun Sep 2 21:27:54 UTC 2012
> On Sept. 2, 2012, 12:04 a.m., Nikita Skovoroda wrote:
> > filters/youtube/youtube-filter.cpp, line 47
> > <http://git.reviewboard.kde.org/r/106083/diff/1/?file=78685#file78685line47>
> >
> > The %1 = url.queryItemValue(QLatin1String("v")) is not escaped here, and can contain ",<,> symbols (and any other complex html).
> >
> > Don't forget that queryItemValue decodes percent-encoded strings.
I'd make a separate variable for the link and escape it using KUrl.
Crude example follows:
html template:
…iframe src=\"%1\"…
url:
KUrl frameUrl(QLatin1String("http://www.youtube.com/embed/%1").arg(id));
inserting the url into the html template:
.arg(QString::fromAscii(frameUrl.toEncoded()))
Or something like that. That should fix things.
> On Sept. 2, 2012, 12:04 a.m., Nikita Skovoroda wrote:
> > filters/youtube/youtube-filter.cpp, line 54
> > <http://git.reviewboard.kde.org/r/106083/diff/1/?file=78685#file78685line54>
> >
> > What happens to http://youtube.com/watch?v=someid ?
>
> Nikita Skovoroda wrote:
> Another possible format: http://youtu.be/someid , this is equivalent to http://www.youtube.com/watch?v=someid
You could add an array/list of structs, for example:
{
QString urlRegexp; // Url for matching and extracting the video id
QString urlSubstitute; // Url for embedding, for example, iframe url
QString html; // The html template that contains the code that needs to be embeded.
}
This way, it would be easy to support different video hosting services and several possible prefixes for each one.
For example, youtube would have:
urlRegexp = "^(?:https?://)?(?:www\.)(?:youtube\.com/watch.*[&\?]v=|youtu\.be/)([^&]+)" // Or something like that, i haven't checked this one, written down without testing.
urlSubstitute = "http://www.youtube.com/embed/%1"
html = "…iframe src=\"%1\"…" // Your html, it's too big to include it here.
> On Sept. 2, 2012, 12:04 a.m., Nikita Skovoroda wrote:
> > filters/youtube/youtube-filter.cpp, line 58
> > <http://git.reviewboard.kde.org/r/106083/diff/1/?file=78685#file78685line58>
> >
> > What happens if «v» parameter is empty?
Parsing the url with regular expressions (see above) would solve this with no extra code involved.
- Nikita
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106083/#review18419
-----------------------------------------------------------
On Aug. 19, 2012, 10:29 a.m., Lasath Fernando wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106083/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2012, 10:29 a.m.)
>
>
> Review request for Telepathy.
>
>
> Description
> -------
>
> Right, so here's a big fat review containing many many things, including all the remaining plugins and a few tweaks to Message.
>
> As per usual, all the code is in my scratch repo: http://quickgit.kde.org/index.php?p=clones%2Fktp-text-ui%2Ffernando%2Fgsoc.git&a=summary
>
> Now, I'm going to take a well earned nap.
>
>
> Diffs
> -----
>
> filters/CMakeLists.txt ee7c23d
> filters/bugzilla/CMakeLists.txt PRE-CREATION
> filters/bugzilla/bugzilla-filter.h PRE-CREATION
> filters/bugzilla/bugzilla-filter.cpp PRE-CREATION
> filters/bugzilla/ktptextui_message_filter_bugzilla.desktop PRE-CREATION
> filters/highlight/CMakeLists.txt PRE-CREATION
> filters/highlight/highlight-filter.h PRE-CREATION
> filters/highlight/highlight-filter.cpp PRE-CREATION
> filters/highlight/ktptextui_message_filter_highlight.desktop PRE-CREATION
> filters/latex/CMakeLists.txt PRE-CREATION
> filters/latex/ktp_message_filter_latex_converter.sh PRE-CREATION
> filters/latex/ktptextui_message_filter_latex.desktop PRE-CREATION
> filters/latex/latex-filter.h PRE-CREATION
> filters/latex/latex-filter.cpp PRE-CREATION
> filters/pipes/CMakeLists.txt PRE-CREATION
> filters/pipes/kcm_ktp_filter_config_pipes.desktop PRE-CREATION
> filters/pipes/ktptextui_message_filter_pipes.desktop PRE-CREATION
> filters/pipes/pipes-config.h PRE-CREATION
> filters/pipes/pipes-config.cpp PRE-CREATION
> filters/pipes/pipes-config.ui PRE-CREATION
> filters/pipes/pipes-delegate.h PRE-CREATION
> filters/pipes/pipes-delegate.cpp PRE-CREATION
> filters/pipes/pipes-filter.h PRE-CREATION
> filters/pipes/pipes-filter.cpp PRE-CREATION
> filters/pipes/pipes-model.h PRE-CREATION
> filters/pipes/pipes-model.cpp PRE-CREATION
> filters/pipes/pipes-prefs.h PRE-CREATION
> filters/pipes/pipes-prefs.cpp PRE-CREATION
> filters/pipes/pipes-prefstest.h PRE-CREATION
> filters/pipes/pipes-prefstest.cpp PRE-CREATION
> filters/substitution/CMakeLists.txt PRE-CREATION
> filters/substitution/kcm_ktp_filter_config_substitution.desktop PRE-CREATION
> filters/substitution/ktptextui_message_filter_substitution.desktop PRE-CREATION
> filters/substitution/substitution-config.h PRE-CREATION
> filters/substitution/substitution-config.cpp PRE-CREATION
> filters/substitution/substitution-config.ui PRE-CREATION
> filters/substitution/substitution-filter.h PRE-CREATION
> filters/substitution/substitution-filter.cpp PRE-CREATION
> filters/substitution/substitution-prefs.h PRE-CREATION
> filters/substitution/substitution-prefs.cpp PRE-CREATION
> filters/youtube/CMakeLists.txt PRE-CREATION
> filters/youtube/ktptextui_message_filter_youtube.desktop PRE-CREATION
> filters/youtube/youtube-filter.h PRE-CREATION
> filters/youtube/youtube-filter.cpp PRE-CREATION
> lib/abstract-message-filter.h 7b60d48
> lib/abstract-message-filter.cpp 2a3a897
> lib/message.h ef9530b
> lib/message.cpp 6db648e
> tests/message-processor-basic-tests.h 7dc99e4
> tests/message-processor-basic-tests.cpp 8546605
>
> Diff: http://git.reviewboard.kde.org/r/106083/diff/
>
>
> Testing
> -------
>
> Wrote/passed/failed unit tests; talked to myself so much to the point I swear I have mild schizophrenia now.
>
>
> Thanks,
>
> Lasath Fernando
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20120902/9e7c95b1/attachment.html>
More information about the KDE-Telepathy
mailing list