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

Revision history for this message
Alexandre Abreu (abreu-alexandre) 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)

ok, I agree to disagree w/ my disagreement, ... this stupid inattention mistake
convinced me,

done (usage of QUrl)

« Back to merge proposal