> On Jan. 10, 2012, 9:36 a.m., Andrea Diamantini wrote: > > 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! :) > > David Narváez wrote: > Uh well, let's negotiate: if I get a rather large mention, I'll do a > better refactoring than if I get a not-so large mention, deal?... :P > > Jokes aside, here are my replies to your comments: > * Session information for an application is stored at > ~/.kde4/shared/config/session, you'll see many files and some of them named > rekonq_* > * I don't think I understand the argument about elementsByTagNamme and an > unneeded for, could you elaborate a bit more on that? > * KMainWindow::readProperties() is protected, makes more sense?
About readProperties... yes, a better check on API lets me understand it makes more sense using it. I guess you are searching for the saveProperties slot to implement desktop/activities restore on crash. This could also lead to a code factorization. Have fun about! ;) - Andrea ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103658/#review9696 ----------------------------------------------------------- On Jan. 10, 2012, 3:42 p.m., David Narváez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103658/ > ----------------------------------------------------------- > > (Updated Jan. 10, 2012, 3:42 p.m.) > > > Review request for rekonq and Tirtha Chatterjee. > > > 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 > >
_______________________________________________ rekonq mailing list [email protected] https://mail.kde.org/mailman/listinfo/rekonq
