Code review comment for lp:~ricmm/unity-mir/extend_lifecycle_mir

Revision history for this message
Albert Astals Cid (aacid) wrote :

src/unity-mir/initialsurfaceplacementstrategy.* seem to not merge cleanly

QTimer::singleShot(3000, this, SLOT(suspend()));
looks dangerous, what if the application gets back to Running state before those 3000 ms? You should stop the timer, in that case, shouldn't you?

the debug log in ApplicationListModel::indexOf is wrong

May make sense to abstract the qprocess code into a function so both startProcess and respawnProcess use it?

I don't understand why you moved m_mainStageApplications->remove(application); into the if, is it because now you can only stop the running app? Wouldn't it make sense to leave it out of the if as a safety mesure?

 QStringList arguments -> const QStringList &arguments

79 + }
80 + else if (m_state == Application::Stopped) {
looks like needs going one line up

Probably you don't need these includes in the header

454 +#include "application_manager.h"
455 +#include "mircommon/mir_toolkit/common.h"
456 +#include <mir/shell/application_session.h>

If you are going to ApplicationManager* appMgr = static_cast<ApplicationManager*>(parent()); in TaskController, i'd feel better if you changed the constructor from TaskController(QObject *parent) to TaskController(ApplicationManager *parent) :

review: Needs Fixing

« Back to merge proposal