[rekonq] Re: Review Request: GCI Taks: Added support for pinning tabs
Pierre Rossi
pierre.rossi at gmail.com
Fri Jan 14 17:29:37 CET 2011
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100305/#review885
-----------------------------------------------------------
src/application.cpp
<http://git.reviewboard.kde.org/r/100305/#comment700>
style fixes could go in a different commit. Could you please split it?
src/application.cpp
<http://git.reviewboard.kde.org/r/100305/#comment702>
style fix (see above)
src/application.cpp
<http://git.reviewboard.kde.org/r/100305/#comment703>
style fix (see above)
src/application.cpp
<http://git.reviewboard.kde.org/r/100305/#comment704>
style fix (see above)
src/application.cpp
<http://git.reviewboard.kde.org/r/100305/#comment701>
maybe a more explicit variable name would help making the code more readable.
src/application.cpp
<http://git.reviewboard.kde.org/r/100305/#comment705>
style fix (see above)
src/application.cpp
<http://git.reviewboard.kde.org/r/100305/#comment706>
style fix (see above)
src/application.cpp
<http://git.reviewboard.kde.org/r/100305/#comment707>
style fix (see above)
src/application.cpp
<http://git.reviewboard.kde.org/r/100305/#comment708>
style fix (see above)
src/mainview.h
<http://git.reviewboard.kde.org/r/100305/#comment709>
why move it ?
src/tabbar.cpp
<http://git.reviewboard.kde.org/r/100305/#comment710>
I'm personally not a big fan of hijacking the tabData thingy for this purpose. Couldn't you keep a private QBitArray member and an accessor isTabPinned(int index) ?
src/tabbar.cpp
<http://git.reviewboard.kde.org/r/100305/#comment713>
the whole concept of using -1 as a default value for the (valid) current index feels a bit weird to me.
src/tabbar.cpp
<http://git.reviewboard.kde.org/r/100305/#comment712>
I'm not sure this case is even worth considering.
src/tabbar.cpp
<http://git.reviewboard.kde.org/r/100305/#comment711>
This could go very wrong. If for any reason indexOf returns -1 (for instance you're not looking at the same email in your pinned GMail), then I fear the whole thing may crash !
A check would be slightly better, but overall I feel the persistance could be handled differently.
src/tabbar.cpp
<http://git.reviewboard.kde.org/r/100305/#comment714>
actually, maybe this should respect the aspect ratio of the tab, just like other previews do. (i.e don't hardcode the height)
src/tabbar.cpp
<http://git.reviewboard.kde.org/r/100305/#comment715>
same remark for tabData()
- Pierre
On Jan. 14, 2011, 2:21 p.m., Furkan Üzümcü wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100305/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2011, 2:21 p.m.)
>
>
> Review request for rekonq.
>
>
> Summary
> -------
>
> Added support for pinning tabs like Firefox 4 and Google Chrome.
> The default preview size was discussed on IRC.
>
>
> Diffs
> -----
>
> src/application.cpp 466a0a4
> src/mainview.h bc8b676
> src/mainview.cpp cbe862f
> src/rekonq.kcfg cda76d8
> src/tabbar.h 4170786
> src/tabbar.cpp fcc7b78
>
> Diff: http://git.reviewboard.kde.org/r/100305/diff
>
>
> Testing
> -------
>
> * Tested and works.
>
>
> Thanks,
>
> Furkan
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/rekonq/attachments/20110114/80711662/attachment-0001.htm
More information about the rekonq
mailing list