[Development] New QUrl reviewing
Thiago Macieira
commented on the basic enablers and I have taken into account a lot of
feedback.
But as the principle of bikeshed goes, no one has reviewed yet the bulk of the
change, which are:
- the URL recoder
- the QUrlQuery class
- the porting of QUrl to the URL recoder and new API
- small fixes done after the porting
I'm asking that reviewers work in the following order:
1) review QUrlQuery for its API, docs and potentially missing tests
https://codereview.qt-project.org/21049
https://codereview.qt-project.org/21057
2) review QUrl for its API, docs and tests
https://codereview.qt-project.org/21058
https://codereview.qt-project.org/21059
https://codereview.qt-project.org/21066
https://codereview.qt-project.org/21067
3) review the recoder as a block, for its functionality and test results, not
for the code itself
https://codereview.qt-project.org/21047
https://codereview.qt-project.org/21048
https://codereview.qt-project.org/21052
In particular, I'm fine if no one approves those three commits. I'll TrustMe
them based on the test results.
4) review the later changes to QUrl
https://codereview.qt-project.org/21060
https://codereview.qt-project.org/21061
https://codereview.qt-project.org/21063
https://codereview.qt-project.org/21066
https://codereview.qt-project.org/21068
5) review the "port" of other code:
https://codereview.qt-project.org/21064
https://codereview.qt-project.org/21493
Most of the changes are only performance improvements since the API retains
compatibility.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
Intel Sweden AB - Registration Number: 556189-6027
Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden
lars....@nokia.com
Lars
On 3/28/12 8:02 PM, "ext Thiago Macieira" <thiago....@intel.com>
wrote:
>_______________________________________________
>Development mailing list
>Devel...@qt-project.org
>http://lists.qt-project.org/mailman/listinfo/development
_______________________________________________
Development mailing list
Devel...@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Thiago Macieira
> I'll try to go through them tonight.
Thanks for the effort. I've gone over all the recommendations and made the
modifications. We're pending further work on:
1) QUrlTwoFlags - a helper class like QFlags but that takes two enums not just
one. I'll simplify this into QFlags if possible and make it generic later.
2) renaming the QUrlQuery methods (i.e., API review). The names were kept
equal to Qt 4's for compatibility reasons, but since this is a new class and
you need to touch your code anyway to use them, we could use better names.
Will do after they code is in.
3) decide if QUrl::errorString() should be translatable
One of the changes has already been staged (the one in qtdeclarative). The
rest have been updated with the fixes. All the commits have two changes except
the first two below: the first is just a rebase because Gerrit required me to
rebase. The second contains the modifications.
To be re-approved or reviewed for the changes:
http://codereview.qt-project.org/21045 only rebase - Gerrit shows no changes
http://codereview.qt-project.org/21046 only rebase - Gerrit shows no changes
http://codereview.qt-project.org/21047 updated the constants to be higher
http://codereview.qt-project.org/21048 only rebase - Gerrit shows no changes
http://codereview.qt-project.org/21052 renamed encodedUtf8ToUcs4 to *Utf16
http://codereview.qt-project.org/21049 two changes requested by Lars
http://codereview.qt-project.org/21057 only rebase - Gerrit shows unrelated
http://codereview.qt-project.org/21056 only rebase - Gerrit shows unrelated
http://codereview.qt-project.org/21058 only rebase - Gerrit shows unrelated
http://codereview.qt-project.org/21060 improvements and test requested by Lars
http://codereview.qt-project.org/21061 only rebase - Gerrit shows unrelated
http://codereview.qt-project.org/21063 only rebase - Gerrit shows unrelated
http://codereview.qt-project.org/21064 only rebase - Gerrit shows unrelated
http://codereview.qt-project.org/21066 changes but Gerrit shows unrelated too
http://codereview.qt-project.org/21067 only rebase - Gerrit shows unrelated
http://codereview.qt-project.org/21068 only rebase - Gerrit shows unrelated
http://codereview.qt-project.org/21751 new change
lars....@nokia.com
Cheers,
Lars
On 3/28/12 11:42 PM, "ext Thiago Macieira" <thiago....@intel.com>
wrote:
>On quarta-feira, 28 de março de 2012 19.08.04, lars....@nokia.com wrote:
marius.st...@nokia.com
--
.marius
bradley...@nokia.com
> Eh, after alpha please?
That will happen already (it's in the api_changes branch).
--
Bradley T. Hughes
bradley...@nokia.com
Thiago Macieira
> All is reviewed. Stage at your convenience ;-)
Staged, re-staged, re-re-staged and once again for good measure, and it's in
:-)
It took two more fixes applied on top -- license headers (some files were added
in September, before we changed our headers) and one behaviour change reversal
caught by QDesktopServices. And one extra attempt due to a network test
timeout failure.
We *really* need the topic branch reviewing feature back in. It's no fun
staging 19, 20, 20 and 21 changes...
Thiago Macieira
> Staged, re-staged, re-re-staged and once again for good measure, and it's
> in :-)
Oh, did I mention I had a QEXPECT_FAIL sneak in due to a feature that David
added to QUrl after I had already completed my work, and to fix that particular
QEXPECT_FAIL, I need to refactor quite a bit of the encoding rules?
Currently, the encoding options are:
- FullyEncoded - as the RFC asks
- DecodeSpace - decode %20 to a space character
- DecodeUnicode - decode unicode sequences to the actual characters
- DecodeUnambiguousDelimiters
- DecodeAllDelimiters
- PrettyDecoded = DecodeUnambiguousDelimiters | DecodeSpace | DecodeUnicode
- MostDecoded = DecodeAllDelimiters | DecodeSpace | DecodeUnicode
The choice of the default has the unfortunate consequence that:
url.setPath("/hash#in#path?");
url.path() == "/hash%23in%23path%3F";
It seems to me that the "pretty decoded" form for the components is actually
the MostDecoded form.
But the MostDecoded form might not be the most interesting form for the full
URL, especially when we consider reserved characters like "\", "^" or "<" and
">".
There's also a side-effect of FullyDecoded being the value 0. If you had:
url.toString();
// equivalent to toString(QUrl::PrettyDecoded)
and you modify it to be now:
url.toString(QUrl::RemovePassword);
oops, that's equivalent to:
url.toString(QUrl::RemovePassword | QUrl::FullyEncoded);
For that reason, I need to rearrange the constants such the default parameter
is a zero. I can't make PrettyDecoded == 0 because otherwise we have the
reverse of the above problem with toEncoded().
Sergio Ahumada
> We *really* need the topic branch reviewing feature back in. It's no fun
> staging 19, 20, 20 and 21 changes...
>
We've fixed some of the problems with the topic branch feature and I'd
like to enable it [1] again after our next release (hopefully next week
or the week after).
After that, I'll create a JIRA task to gather changes or new features
that might be required. Bugs should be filed separately.
Cheers,
[1]
- for all projects ?
- for some projects as a pilot ?
- for a dummy project so people can test it before going live ?
--
Sergio Ahumada
Stephen Kelly
On Thursday, March 29, 2012 23:31:38 Thiago Macieira wrote:
> We really need the topic branch reviewing feature back in. It's no fun
> staging 19, 20, 20 and 21 changes...
Did you consider staging them a few commits at a time?
Thanks,
--
Stephen Kelly <stephe...@kdab.com> | Software Engineer
KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company
www.kdab.com || Germany +49-30-521325470 || Sweden (HQ) +46-563-540090
KDAB - Qt Experts - Platform-Independent Software Solutions
Thiago Macieira
> On Thursday, March 29, 2012 23:31:38 Thiago Macieira wrote:
> > We really need the topic branch reviewing feature back in. It's no fun
> > staging 19, 20, 20 and 21 changes...
>
> Did you consider staging them a few commits at a time?
Yes. The tests might not pass. In fact, the very first commit had a test
failure which was fixed later on and I wasn't about to figure out what had fixed
it.
It's all or nothing. That's what the topic branch feature is for.
Stephen Kelly
On Friday, March 30, 2012 12:45:44 Thiago Macieira wrote:
> On sexta-feira, 30 de março de 2012 17.00.15, Stephen Kelly wrote:
> > On Thursday, March 29, 2012 23:31:38 Thiago Macieira wrote:
> > > We really need the topic branch reviewing feature back in. It's no
> > > fun
> > > staging 19, 20, 20 and 21 changes...
> >
> > Did you consider staging them a few commits at a time?
>
> Yes. The tests might not pass. In fact, the very first commit had a test
> failure which was fixed later on and I wasn't about to figure out what had
> fixed it.
>
> It's all or nothing. That's what the topic branch feature is for.
That means in the future anyone debugging a bug introduced in one of the patches will also get unrelated failures in the unit tests, or maybe won't be able to bisect.
Also definitely not ideal, but you're more likely to be debugging it in the future than me...
Thiago Macieira
> On Friday, March 30, 2012 12:45:44 Thiago Macieira wrote:
> > On sexta-feira, 30 de março de 2012 17.00.15, Stephen Kelly wrote:
> > > On Thursday, March 29, 2012 23:31:38 Thiago Macieira wrote:
> > > > We really need the topic branch reviewing feature back in. It's no
> > > > fun
> > > > staging 19, 20, 20 and 21 changes...
> > >
> > > Did you consider staging them a few commits at a time?
> >
> > Yes. The tests might not pass. In fact, the very first commit had a test
> > failure which was fixed later on and I wasn't about to figure out what had
> > fixed it.
> >
> > It's all or nothing. That's what the topic branch feature is for.
>
> That means in the future anyone debugging a bug introduced in one of the
> patches will also get unrelated failures in the unit tests, or maybe won't
> be able to bisect.
Yes, then they skip it.
> Also definitely not ideal, but you're more likely to be debugging it in the
> future than me...
If I can, I try to make sure that each commit is testable on its own.
However, I will sacrifice testing for atomic changes. I don't like having big
changes doing lots of things because it's hard to understand how certain
changes came by.
Stephen Kelly
On Saturday, March 31, 2012 13:34:52 Thiago Macieira wrote:
> On sábado, 31 de março de 2012 18.14.50, Stephen Kelly wrote:
> > On Friday, March 30, 2012 12:45:44 Thiago Macieira wrote:
> > > On sexta-feira, 30 de março de 2012 17.00.15, Stephen Kelly wrote:
> > > > On Thursday, March 29, 2012 23:31:38 Thiago Macieira wrote:
> > > > > We really need the topic branch reviewing feature back in.
> > > > > It's no
> > > > > fun
> > > > > staging 19, 20, 20 and 21 changes...
> > > >
> > > > Did you consider staging them a few commits at a time?
> > >
> > > Yes. The tests might not pass. In fact, the very first commit had a
> > > test failure which was fixed later on and I wasn't about to figure
> > > out what had fixed it.
> > >
> > > It's all or nothing. That's what the topic branch feature is for.
> >
> > That means in the future anyone debugging a bug introduced in one of the
> > patches will also get unrelated failures in the unit tests, or maybe
> > won't be able to bisect.
>
> Yes, then they skip it.
If the bug was introduced in one of your 19, 20, 21 patches, then skipping all of them won't do much good.
>
> > Also definitely not ideal, but you're more likely to be debugging it in
> > the future than me...
>
> If I can, I try to make sure that each commit is testable on its own.
>
> However, I will sacrifice testing for atomic changes. I don't like having
> big changes doing lots of things because it's hard to understand how
> certain changes came by.
Agreed. I certainly never suggested you should squash commits together.
I think making commits which are not testable on their own should be discouraged.
Thiago Macieira
> > If I can, I try to make sure that each commit is testable on its own.
> >
> >
> >
> > However, I will sacrifice testing for atomic changes. I don't like having
> > big changes doing lots of things because it's hard to understand how
> > certain changes came by.
>
> Agreed. I certainly never suggested you should squash commits together.
Which, incidentally, is another source of non-testability.
The original QUrl series contained 23 patches. Besides the basics of working
on IP addresses, etc., it started by introducing the URL recoder, then
adapting it according to some discussions on this ML; then it added the
QUrlQuery class, full with passing tests, and then proceeded to change the
recoder to be more efficient given the code I had written for QUrlQuery. Then,
given the discussion here, we decided that QUrlQuery should keep the order of
its elements instead of letting QHash order them.
When I uploaded the changes, one of the reviews requested the QUrlQuery
addition, documentation and then later change of behaviour should be squashed.
Which I did, but it necessitated moving the optimisations and improvements to
the recoder before QUrlQuery was added.
I've never tested if that reordering introduced failures in existing tests.