Code review comment for lp:~paulliu/unity-mir/logout

In src/modules/Unity/Application/dbuslogout.cpp

"""
140 +#include "application_manager.h"
"""

Should be removed

-----------------------------------------

In src/modules/Unity/Application/dbuslogout.h:

"""
208 +class ApplicationManager;
"""

Should be removed.

"""
227 +private:
228 + ApplicationManager *m_applicationManager;
"""

Should be removed.

------------------------------------------

Is this com.canonical.Unity.Session inteface documented elsewhere? If not I would like to see it documented in src/modules/Unity/Application/dbuslogout.h. It's not clear to me how it's supposed to be used.

"""
Q_SCRIPTABLE void logoutRequested(bool b);
"""

What's that boolean for?

What's the workflow here? RequestLogout() emits logoutRequested() and Logout() emits logoutReady(). Naming is a bit incosistent. What's the meaning and difference between RequestLogout() and Logout()? ApplicationManager is using only of the the two...

---------------------------------------------

ApplicationManager knowing about the logout concept feels out of place. As it name says, it should know only about managing apps. I believe it should be unity8 code doing the connection between the two.

AppliationManger::unityLogout()

Poorly named to say the least (check naming conventions for signals). I believe a clearer separation of tasks and responsibilities would be having something like the following in unity8 code:

DBusLogout {
    onLogoutRequested {
       ApplicationManager.stopAllApplications()
       Qt.quit();
    }
}

Which brings to mind the name of this class: DBusLogout. As it's implementing the com.canonical.Unity.Session service, I would clearer if it was named after it. After all, com.canonical.Unity.Session should be about more than just a Logout method, right? Thus:

UnitySessionDBusService {
    onLogoutRequested {
       ApplicationManager.stopAllApplications()
       Qt.quit();
    }
}