[rekonq] Re: Review Request: User Agent Switcher.

[Andrea Diamantini]

> On Jan. 12, 2011, 11:33 p.m., Benjamin Poulain wrote:
> > src/mainwindow.cpp, line 1422
> > <http://git.reviewboard.kde.org/r/100373/diff/1/?file=6725#file6725line1422>
> >
> >     Why do you convert the data as String? You could use integers with QVariant and avoid all the conversions back and forth.
> >     
> >     I am also not a fan of this special case at -1. I see that becoming a problem when someone will forget and use that index in a data structure.

Right about conversion. Fixed.
I cannot see the case of using -1 in some data structure. Anyway everything depends on the API choice. Thinking a bit better about it.


> On Jan. 12, 2011, 11:33 p.m., Benjamin Poulain wrote:
> > src/mainwindow.cpp, lines 1534-1542
> > <http://git.reviewboard.kde.org/r/100373/diff/1/?file=6725#file6725line1534>
> >
> >     Since you are using ::exec(), the dialog cannot exist behond this function. So you could just allocated it on the stack.

This code comes from here: http://www.kdedevelopers.org/node/3919


> On Jan. 12, 2011, 11:33 p.m., Benjamin Poulain wrote:
> > src/useragent/useragentwidget.cpp, lines 59-61
> > <http://git.reviewboard.kde.org/r/100373/diff/1/?file=6730#file6730line59>
> >
> >     You could just not have a destructor.

Yeah! Sometimes automatism go beyond needing. Fixed in next patch.


- Andrea


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


On Jan. 12, 2011, 10:56 p.m., Andrea Diamantini wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100373/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2011, 10:56 p.m.)
> 
> 
> Review request for rekonq.
> 
> 
> Summary
> -------
> 
> User Agent Switcher. 
> 
> This patch should implement the UA switcher ability for rekonq. This is a first implementation, but it seems working quite well. It is based on KDE UA management and should be fully compatible with konqueror's one i.e. it should be possible using alternatively rekonq and konqueror sharing the same setting.
> 
> Anyway, this is a different implementation from the konqueror's one. Simpler and based on the idea
> of a future moving to a plugin.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt f0310b4d91229a6d8322c163069165b940c41efe 
>   src/application.cpp 95aa9cf0b0a40006c5c0e1663acce506e4c4e123 
>   src/mainwindow.h 33fd20212b9bb69d9450298e4e43324885f8bdf0 
>   src/mainwindow.cpp f662d7aa77d8045372c43514c6a04ea497a9afbc 
>   src/useragent/useragentinfo.h PRE-CREATION 
>   src/useragent/useragentinfo.cpp PRE-CREATION 
>   src/useragent/useragentsettings.ui PRE-CREATION 
>   src/useragent/useragentwidget.h PRE-CREATION 
>   src/useragent/useragentwidget.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/100373/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrea
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/rekonq/attachments/20110119/b573c7b2/attachment-0001.htm