Re: [Development] New QUrl reviewing
On sábado, 31 de março de 2012 19.02.32, Stephen Kelly wrote: 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. -- 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 signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] New QUrl reviewing
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 stephen.ke...@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 signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] New QUrl reviewing
All is reviewed. Stage at your convenience ;-) Cheers, Lars On 3/28/12 11:42 PM, ext Thiago Macieira thiago.macie...@intel.com wrote: On quarta-feira, 28 de março de 2012 19.08.04, lars.kn...@nokia.com wrote: 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 -- 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 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] New QUrl reviewing
Eh, after alpha please? -- .marius -Original Message- From: development-bounces+marius.storm-olsen=nokia.com@qt- project.org [mailto:development-bounces+marius.storm- olsen=nokia@qt-project.org] On Behalf Of Knoll Lars (Nokia-MP/Oslo) Sent: Thursday, March 29, 2012 6:56 AM To: thiago.macie...@intel.com; development@qt-project.org Subject: Re: [Development] New QUrl reviewing All is reviewed. Stage at your convenience ;-) Cheers, Lars On 3/28/12 11:42 PM, ext Thiago Macieira thiago.macie...@intel.com wrote: On quarta-feira, 28 de março de 2012 19.08.04, lars.kn...@nokia.com wrote: 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 -- 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 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] New QUrl reviewing
On quinta-feira, 29 de março de 2012 11.55.47, lars.kn...@nokia.com wrote: 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 - 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 signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] New QUrl reviewing
On quinta-feira, 29 de março de 2012 23.31.38, Thiago Macieira wrote: 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(). -- 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 signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
[Development] New QUrl reviewing
The new QUrl has been up for review for several days now. Lots of people have 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 signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] New QUrl reviewing
I'll try to go through them tonight. Lars On 3/28/12 8:02 PM, ext Thiago Macieira thiago.macie...@intel.com wrote: The new QUrl has been up for review for several days now. Lots of people have 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 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] New QUrl reviewing
On quarta-feira, 28 de março de 2012 19.08.04, lars.kn...@nokia.com wrote: 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 -- 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 signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
