Code review comment for lp://staging/~abreu-alexandre/webbrowser-app/intent

Revision history for this message
Olivier Tilloy (osomon) wrote :

> > Why not make the parameter of parseIntentUri(…) a QUrl ?
> > This would make it much easier to parse the URI, without the need for
> > regexpes. E.g.:
> >
> > QUrl url("intent://…");
> >
> > qDebug() << "URL:" << url;
> > qDebug() << "scheme:" << url.scheme();
> > qDebug() << "host:" << url.host();
> > qDebug() << "path:" << url.path();
> > qDebug() << "query:" << url.query();
> > qDebug() << "fragment:" << url.fragment();
> >
> > QStringList fragments = url.fragment().split(";");
> > assert(fragments.takeFirst() == "Intent");
> > assert(fragments.takeLast() == "end");
> > QMap<QString, QString> tokens;
> > Q_FOREACH(const QString& fragment, fragments) {
> > QStringList token = fragment.split("=");
> > assert(token.size() == 2);
> > tokens.insert(token[0], token[1]);
> > }
> > qDebug() << "tokens:" << tokens;
>
> I'd agree, except that it also brings extra logic to reconstruct
> the intent URI parts:
> - path + query (if any),
> - host (if any) and path,
> etc.
> plus some other things,
>
> which in the end makes it also a bit convoluted imo,

Fair enough (I disagree but let’s agree to disagree). A couple of comments on the logic though:

 - the intentRe regexp should end with the ";end;" token
 - the assignment of result.host and result.uriPath will fail if s has more than 2 items (e.g. if the path is several levels deep, like example.org/example/path/to/some/resource)

review: Needs Fixing

« Back to merge proposal