[rekonq] Re: Review Request: Icon for add opensearch engine
Benjamin Poulain
ikipou at gmail.com
Wed Dec 8 19:17:52 CET 2010
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100064/#review468
-----------------------------------------------------------
src/opensearch/opensearchengine.cpp
<http://git.reviewboard.kde.org/r/100064/#comment329>
This line is hard to read to me.
I would do that in 2 step:
1) remove the first dash:
const int firstDashPosition = country.indexOf(QL1C('-'));
if (firstDash >= 0)
country.remove(firstDashPosition, 1)
2) lower the string
country = country.lower()
It is a bit longuer, but clearer IMHO.
src/opensearch/opensearchmanager.cpp
<http://git.reviewboard.kde.org/r/100064/#comment330>
QL1S() missing here?
src/opensearch/opensearchmanager.cpp
<http://git.reviewboard.kde.org/r/100064/#comment331>
Please put some explanation in the TODO, like
// TODO: handle charset of the provider.
This limits the chance of having the TODO out of sync with the code.
src/opensearch/opensearchmanager.cpp
<http://git.reviewboard.kde.org/r/100064/#comment332>
QFile is a bit overkill just to find if a file exists. QFile is a QIODevice->QObject.
QFileInfo::exists() should do the job without so much overhead.
There are also QL1S() missing I think.
src/urlbar/listitem.cpp
<http://git.reviewboard.kde.org/r/100064/#comment333>
Something does not seems right here.
if (item.type & UrlSearchItem::Suggestion) {
[...]
if (item.type & UrlSearchItem::Suggestion) {
The second if is always true. You added something in the else, I don't think it is ever reached.
src/urlbar/urlbar.h
<http://git.reviewboard.kde.org/r/100064/#comment334>
Not your fault, but WTF are those names? :-D
At least "SearchEngine" is clear :)
src/urlbar/urlbar.cpp
<http://git.reviewboard.kde.org/r/100064/#comment335>
QL1S()?
src/urlbar/urlresolver.cpp
<http://git.reviewboard.kde.org/r/100064/#comment336>
Please put the if() on two lines. Single line branching are very hard to read.
src/urlbar/webshortcutwidget.h
<http://git.reviewboard.kde.org/r/100064/#comment338>
Copy paste? Isn't your name supposed to be here?
src/urlbar/webshortcutwidget.h
<http://git.reviewboard.kde.org/r/100064/#comment339>
This constructor should be explicit.
src/urlbar/webshortcutwidget.h
<http://git.reviewboard.kde.org/r/100064/#comment337>
Space between & and pos for the third argument.
src/urlbar/webshortcutwidget.cpp
<http://git.reviewboard.kde.org/r/100064/#comment340>
Same as for the header, your name should probably be here.
src/webtab.h
<http://git.reviewboard.kde.org/r/100064/#comment341>
"const QPoint &" for the argument.
Just a suggestion: something we do in WebKit is sort the #include by alphabetical order.
I have not much time, I just quickly went over the code for the obvious things. I'll do a real review later, your patch is HUGE :)
- Benjamin
On 2010-12-08 08:20:34, Lionel Chauvin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100064/
> -----------------------------------------------------------
>
> (Updated 2010-12-08 08:20:34)
>
>
> Review request for rekonq.
>
>
> Summary
> -------
>
> Icon at the right of the urlbar that allows to add an opensearch engine
>
>
> Diffs
> -----
>
> "a/src/data/recherche_de_vid/303/251os_youtube.xml" PRE-CREATION
> "a/src/data/wikip/303/251dia_(fr).xml" PRE-CREATION
> src/CMakeLists.txt f0310b4
> src/application.cpp 95aa9cf
> src/data/7digitalcom.xml PRE-CREATION
> src/data/CMakeLists.txt 5f49994
> src/data/beolingus.xml PRE-CREATION
> src/data/bliptv.xml PRE-CREATION
> src/data/bugzilla.xml PRE-CREATION
> src/data/citeseerx.xml PRE-CREATION
> src/data/cpan_search.xml PRE-CREATION
> src/data/de-en_beolingus.xml PRE-CREATION
> src/data/dictfr.xml PRE-CREATION
> src/data/duck_duck_go.xml PRE-CREATION
> src/data/ecosia.xml PRE-CREATION
> src/data/facebook.xml PRE-CREATION
> src/data/flickr.xml PRE-CREATION
> src/data/github.xml PRE-CREATION
> src/data/identica_people_search.xml PRE-CREATION
> src/data/kde_techbase.xml PRE-CREATION
> src/data/kde_techbase_(en).xml PRE-CREATION
> src/data/kde_userbase.xml PRE-CREATION
> src/data/kde_userbase_(en).xml PRE-CREATION
> src/data/leo_deu-eng.xml PRE-CREATION
> src/data/leo_deu-fra.xml PRE-CREATION
> src/data/portail_lexical_-_cnrtl.xml PRE-CREATION
> src/data/urban_dictionary.xml PRE-CREATION
> src/data/voila.xml PRE-CREATION
> src/data/wikia.xml PRE-CREATION
> src/data/wikia_(en).xml PRE-CREATION
> src/data/wikipedia.xml 4e15208
> src/data/wiktionary.xml PRE-CREATION
> src/data/wiktionary_(en).xml PRE-CREATION
> src/data/wolfram_alpha.xml PRE-CREATION
> src/data/wr_english-french.xml PRE-CREATION
> src/data/wr_english-italian.xml PRE-CREATION
> src/data/wr_english-spanish.xml PRE-CREATION
> src/data/wr_english.xml PRE-CREATION
> src/data/wr_french-english.xml PRE-CREATION
> src/data/wr_italian-english.xml PRE-CREATION
> src/data/wr_spanish-english.xml PRE-CREATION
> src/data/yippy.xml PRE-CREATION
> src/mainwindow.cpp c9863f0
> src/opensearch/opensearchengine.cpp 9d5e866
> src/opensearch/opensearchmanager.h 4f42b4a
> src/opensearch/opensearchmanager.cpp e5afc14
> src/opensearch/suggestionparser.cpp 4ed13e1
> src/settings/generalwidget.cpp 642813a
> src/settings/settingsdialog.cpp a3b4f97
> src/urlbar/listitem.h f5f1153
> src/urlbar/listitem.cpp f29a0e9
> src/urlbar/urlbar.h 67a693c
> src/urlbar/urlbar.cpp 1f4a036
> src/urlbar/urlresolver.cpp 57d6ca6
> src/urlbar/webshortcutwidget.h PRE-CREATION
> src/urlbar/webshortcutwidget.cpp PRE-CREATION
> src/webtab.h 12f528d
> src/webtab.cpp bac73fe
>
> Diff: http://git.reviewboard.kde.org/r/100064/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Lionel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/rekonq/attachments/20101208/62d76622/attachment-0001.htm
More information about the rekonq
mailing list