[rekonq] Re: Review Request: Icon for add opensearch engine

Lionel Chauvin megabigbug at yahoo.fr
Wed Dec 8 20:59:14 CET 2010 


> On 2010-12-08 18:17:58, Benjamin Poulain wrote:
> > src/urlbar/webshortcutwidget.h, line 2
> > <http://git.reviewboard.kde.org/r/100064/diff/4/?file=4850#file4850line2>
> >
> >     Copy paste? Isn't your name supposed to be here?

This class come from konqueror's searchbar plug-in. Fredy Yanardi is the author. I modified the class, so I should add my name too.


> On 2010-12-08 18:17:58, Benjamin Poulain wrote:
> > src/urlbar/listitem.cpp, lines 636-639
> > <http://git.reviewboard.kde.org/r/100064/diff/4/?file=4846#file4846line636>
> >
> >     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.

true.
it is a bad merge resolution.


> On 2010-12-08 18:17:58, Benjamin Poulain wrote:
> > src/opensearch/opensearchengine.cpp, line 61
> > <http://git.reviewboard.kde.org/r/100064/diff/4/?file=4839#file4839line61>
> >
> >     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.

ok


> On 2010-12-08 18:17:58, Benjamin Poulain wrote:
> > src/opensearch/opensearchmanager.cpp, lines 241-243
> > <http://git.reviewboard.kde.org/r/100064/diff/4/?file=4841#file4841line241>
> >
> >     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.

ok I must study that.


> On 2010-12-08 18:17:58, Benjamin Poulain wrote:
> > src/opensearch/opensearchmanager.cpp, line 217
> > <http://git.reviewboard.kde.org/r/100064/diff/4/?file=4841#file4841line217>
> >
> >     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.
> >

ok


> On 2010-12-08 18:17:58, Benjamin Poulain wrote:
> > src/urlbar/webshortcutwidget.h, line 33
> > <http://git.reviewboard.kde.org/r/100064/diff/4/?file=4850#file4850line33>
> >
> >     This constructor should be explicit.

ok


> On 2010-12-08 18:17:58, Benjamin Poulain wrote:
> > src/urlbar/webshortcutwidget.h, line 35
> > <http://git.reviewboard.kde.org/r/100064/diff/4/?file=4850#file4850line35>
> >
> >     Space between & and pos for the third argument.

ok


> On 2010-12-08 18:17:58, Benjamin Poulain wrote:
> > src/webtab.h, line 86
> > <http://git.reviewboard.kde.org/r/100064/diff/4/?file=4852#file4852line86>
> >
> >     "const QPoint &" for the argument.

ok


- Lionel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100064/#review468
-----------------------------------------------------------


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/3c3dc83e/attachment-0001.htm

More information about the rekonq mailing list