[Konsole-devel] [Bug 152761] No session management in KDE 4 - Sessions / directory / tabs not restored on login

Robert Knight robertknight at gmail.com
Sun Sep 14 23:17:02 UTC 2008


http://bugs.kde.org/show_bug.cgi?id=152761





--- Comment #37 from Robert Knight <robertknight gmail com>  2008-09-15 01:17:00 ---
Hi Stefan,

Comments on the session management part of the patch:

Functional comments on the patch:

+void MainWindow::saveGlobalProperties(KConfig* config)
+{
+    SessionManager::instance()->saveSessions(config);
+}
+
+void MainWindow::readGlobalProperties(KConfig* config)
+{
+    SessionManager::instance()->restoreSessions(config);
+}
+

An application may have multiple main windows, each window containing multiple
views (TerminalDisplay instances).  Each view is associated with only one
session but each session may be associated with multiple views.  This code
would load all the sessions multiple times, once for each window restored.

I suggest that we simplify things to begin with by only trying to restore the
first view for each session.  The process to save would be:

- Save all sessions
- For each window
-   -> Save the window properties (geometry etc.) (Use KMainWindow base class
for this)
-   -> Save the association between window and session in order.  Where a
session has multiple views consider only the first one.

To load:

- Load all sessions (but do not run them)
- For each window
-   -> Load the window properties (use KMainWindow base class for this)
-   -> Create one view for each session in that window, using the saved order
- Run all sessions (using session->run() )

+       // this should not happen
+       Q_ASSERT(id == 0);
+       return 0;

This assert is not needed, you have one at the start of the function with the
opposite condition.  I would rewrite this sort of loop with just one exit
point:

Session* foundSession = 0;
foreach(Session* session,sessions)
  if (session->sessionId() == id)
    foundSession = session;
Q_ASSERT(foundSession);
return foundSessionl


Stylistic comments:

Please try to avoid mixing formatting and functional changes in the same patch.
 It makes it more difficult to examine the more important functional changes.

+    // second: all other sessions, in random order
+    // we don't want to have sessions restored that are not connected...

Please do not end comments with '...'.  It implies, to me, that the comment is
only telling the reader part of what they need to know and leaving the rest as
guesswork.  

+    static bool first = true;

+    SessionManager *mgr = SessionManager::instance();
+    QString profile = group.readPathEntry("Default Profile", QString());
+    Profile::Ptr pp = mgr->defaultProfile();
+    if (!profile.isEmpty()) pp = mgr->loadProfile(profile);

Use of abbreviations in variable names should generally be avoided.  They can
lead to misunderstandings and consequently bugs in longer functions.  Very
common abbreviations (ptr,src,dest,len,pos) may be used.

+       // session management
+       virtual void saveProperties(KConfigGroup& group);
+       virtual void readProperties(const KConfigGroup& group);
+       virtual void saveGlobalProperties(KConfig* config);
+       virtual void readGlobalProperties(KConfig* config);

When a group of virtual functions are re-implementations from a base class I
find it helpful if there is a comment at the top making that clear.  In this
case just delete the 'session management' comment at the line above to do that. 


-- 
Configure bugmail: http://bugs.kde.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the konsole-devel mailing list