[rekonq] Re: Review Request: Restore the tab's history when restoring a tab/session

[Anton Kreuzkamp]

> On April 7, 2011, 10:34 p.m., Pierre Rossi wrote:
> > src/sessionmanager.cpp, line 91
> > <http://git.reviewboard.kde.org/r/100604/diff/3/?file=13941#file13941line91>
> >
> >     is base64 really necessary if it's already in a CDATA section within a xml document ?

the QString(QByteArray) constructor reads out the qbytearray until the first byte with the value 0. But somehow every second byte in the history-bytearray is 0, so the only way I see to get the whole array is base64.


> On April 7, 2011, 10:34 p.m., Pierre Rossi wrote:
> > src/sessionmanager.cpp, line 186
> > <http://git.reviewboard.kde.org/r/100604/diff/3/?file=13941#file13941line186>
> >
> >     I'm a bit confused here... Did you mean to call the slot (without emit) or emit the currentChanged signal ?

the emit is definitely a mistake. Thanks for pointing out :)


> On April 7, 2011, 10:34 p.m., Pierre Rossi wrote:
> > src/sessionmanager.cpp, line 201
> > <http://git.reviewboard.kde.org/r/100604/diff/3/?file=13941#file13941line201>
> >
> >     Nice catch on this refactoring, but I feel it could go in a separate micro-commit, since it is not specially tied to this change.
> >     (r=me for that commit if you feel like doing so)

ok.


> On April 7, 2011, 10:34 p.m., Pierre Rossi wrote:
> > src/sessionmanager.cpp, line 219
> > <http://git.reviewboard.kde.org/r/100604/diff/3/?file=13941#file13941line219>
> >
> >     how about using something like this instead:
> >     foreach (const QDomElement element, document.elementsByTagName("tab"))

elementsByTagName() returns a QDomNodeList which isn't a valid type for Q_FOREACH.


> On April 7, 2011, 10:34 p.m., Pierre Rossi wrote:
> > src/history/historymanager.h, line 129
> > <http://git.reviewboard.kde.org/r/100604/diff/3/?file=13936#file13936line129>
> >
> >     It feels "url" would work well for this field's name, and a bunch of lines above would get to stay the same. Otherwise title should be called currentTitle for consistency, but since it's quite obvious I think you can drop the "current" altogether.

ok, I'm fine with that.


> On April 7, 2011, 10:34 p.m., Pierre Rossi wrote:
> > src/mainview.cpp, line 104
> > <http://git.reviewboard.kde.org/r/100604/diff/3/?file=13938#file13938line104>
> >
> >     do we really need this all foreach loop ? It doesn't seem very likely to me that two closed tabs will have the same history...

I don't see a better way currently.. (I'll have a look at it again)


> On April 7, 2011, 10:34 p.m., Pierre Rossi wrote:
> > src/mainview.cpp, line 654
> > <http://git.reviewboard.kde.org/r/100604/diff/3/?file=13938#file13938line654>
> >
> >     Is this really necessary ? (considering the takeFirst() above)

*just recognizes that takeFirst() removes the item already*
no it's not necessary.


- Anton


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


On April 7, 2011, 6:48 p.m., Anton Kreuzkamp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100604/
> -----------------------------------------------------------
> 
> (Updated April 7, 2011, 6:48 p.m.)
> 
> 
> Review request for rekonq.
> 
> 
> Summary
> -------
> 
> Restore the history of the tab when restoring a tab from the list of closed tabs, by open last closed tab or by restoring the session.
> Therefore I ported the session-file to xml, as it allows more complex session-files (which is also needed for multiple sessions support).
> (Doesn't require a qtwebkit-patch anymore)
> 
> 
> Diffs
> -----
> 
>   src/application.cpp 1fe936f 
>   src/history/historymanager.h eb8d78d 
>   src/mainview.h acc2d8c 
>   src/mainview.cpp b34acc3 
>   src/newtabpage.cpp a598066 
>   src/sessionmanager.h 2297290 
>   src/sessionmanager.cpp 68bc635 
>   src/tabbar.cpp 1ab357f 
> 
> Diff: http://git.reviewboard.kde.org/r/100604/diff
> 
> 
> Testing
> -------
> 
> tested and everything seems to work fine.
> 
> 
> Thanks,
> 
> Anton
> 
>

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