[rekonq] Review Request: Implement (User) Session Restore

Andrea Diamantini adjam7 at gmail.com
Tue Jan 10 09:36:32 UTC 2012


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


Hi David, and many thanks for your work. There are a lot of things I like in your patch and some that need discussion. Anyway, you surely let me rethink/work about session restore :)
First, I think that the "improvements" here came basically from moving the check where application "canBeRestored" in main, where app can read eventual config files (? where are them?) saved from kapplication class.
I like the check about mainwindows restore name added, but I think the code in SessionManager::restoreMainWindow can be improved removing the unuseful for and using some QDom* magic (I guess "elementsByTagName" is our friend here).

I don't like the use of KMainWindow::restore(int) with its indirect call the readProperties. Who ensure a general public method like "readProperties" is used just on restore? No one but actual code. So, I vote for implementing a local KMainWindow::restoreMe() (or whatever name you prefer) calling SessionManager::restoreMainWindow(const QString &windowName).

Given that, code will be perfect for me and I'll love you can take care also about refactoring it. You'll have a special mention in rekonq about dialog for all this! :)

- Andrea Diamantini


On Jan. 9, 2012, 7:11 a.m., David Narváez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103658/
> -----------------------------------------------------------
> 
> (Updated Jan. 9, 2012, 7:11 a.m.)
> 
> 
> Review request for rekonq.
> 
> 
> Description
> -------
> 
> This patch implements session management (see http://techbase.kde.org/Development/Tutorials/Session_Management). Most of the ideas are taken from Konsole, which is also a KUniqueApplication but manages session restoring correctly. Notice that the new code in Session Manager triplicates some code from other functions (mostly dealing with loading the session DOM Document and restoring tabs), so I think that if this patch is accepted, a refactoring of the SessionManager code is in order (or maybe do the refactoring before this patch?). I can take a shot at the refactoring if needed.
> 
> 
> Diffs
> -----
> 
>   src/main.cpp a9082d7 
>   src/mainwindow.h 04a59a1 
>   src/mainwindow.cpp 98936e9 
>   src/sessionmanager.h d0d7efd 
>   src/sessionmanager.cpp 20b3501 
>   src/application.cpp a09fc26 
> 
> Diff: http://git.reviewboard.kde.org/r/103658/diff/diff
> 
> 
> Testing
> -------
> 
> Test #1: Desktop restoration
> 1) Open Rekonq in one Desktop
> 2) Change to another desktop
> 3) Log out and back in
> 
> Rekonq should not appear in the desktop you are at, but in the desktop you were when you called it.
> 
> Test #2: Activities restoration
> 1) Open two activities
> 2) Open two different Rekonq windows, one in each activity, with different URLs
> 3) Log out and back in
> 
> Rekonq should be restore in both activities with the URLs you loaded before logging out
> 
> Other tests:
> 1) Do Test #2, then open a new rekonq window - this tests changes in the Application::newInstance() method.
> 2) Crash the session (e.g., killall rekonq) and open rekonq again - in this case no desktop/activities restoration should take place (haven't figured out how to do this)
> 
> 
> Thanks,
> 
> David Narváez
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/rekonq/attachments/20120110/3af52642/attachment.html>


More information about the rekonq mailing list